From 4487a14e7cce62e4fb6c1ebddac5f3e713c061de Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Sun, 20 Sep 2020 14:53:51 +0200 Subject: loplugin:xmlimport check for bad conversions to fastparser add a check for classes which have been partly converted to fastparser, but not completedly. This is to help me when I convert stuff. and it uncovers a bug introduced with commit 998308c363dfad03143591aa18256d2669b4da11 use more FastParser in SvXMLStylesContext Change-Id: Ib50e7136da10a1a7a346102aa47efef2f543e2ac Reviewed-on: https://gerrit.libreoffice.org/c/core/+/102669 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/test/xmlimport.cxx | 36 +++++++- compilerplugins/clang/xmlimport.cxx | 152 +++++++++++++++++++++---------- 2 files changed, 141 insertions(+), 47 deletions(-) (limited to 'compilerplugins') diff --git a/compilerplugins/clang/test/xmlimport.cxx b/compilerplugins/clang/test/xmlimport.cxx index 965d4936e41c..36230ffdb2d8 100644 --- a/compilerplugins/clang/test/xmlimport.cxx +++ b/compilerplugins/clang/test/xmlimport.cxx @@ -1,4 +1,4 @@ -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ /* * This file is part of the LibreOffice project. * @@ -12,6 +12,13 @@ // Cannot include this, makes clang crash //#include "xmloff/xmlimp.hxx" +#include + +namespace com::sun::star::xml::sax +{ +class XAttributeList; +} + class SvXMLImportContext { public: @@ -20,6 +27,10 @@ public: virtual void createFastChildContext() {} virtual void startFastElement() {} virtual void endFastElement() {} + + virtual void StartElement(const css::uno::Reference&) {} + virtual void EndElement() {} + virtual void Characters(const OUString&) {} }; class Test1 : public SvXMLImportContext @@ -51,4 +62,27 @@ public: virtual void endFastElement() override; }; +class Test5 : public SvXMLImportContext +{ +public: + // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}} + virtual void + StartElement(const css::uno::Reference& xAttrList) override; + // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}} + virtual void EndElement() override; + // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}} + virtual void Characters(const OUString&) override; +}; + +// no warning expected +class Test6 : public SvXMLImportContext +{ +public: + Test6(sal_uInt16, const OUString&); + virtual void + StartElement(const css::uno::Reference& xAttrList) override; + virtual void EndElement() override; + virtual void Characters(const OUString&) override; +}; + /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ diff --git a/compilerplugins/clang/xmlimport.cxx b/compilerplugins/clang/xmlimport.cxx index bdb41f616859..d6b1aa78e325 100644 --- a/compilerplugins/clang/xmlimport.cxx +++ b/compilerplugins/clang/xmlimport.cxx @@ -52,81 +52,141 @@ public: bool VisitCXXMethodDecl(const CXXMethodDecl*); }; -static bool containsStartFastElementMethod(const CXXRecordDecl* cxxRecordDecl, - bool& rbFoundImportContext) +static bool containsStartFastElementMethod(const CXXRecordDecl* cxxRecordDecl) { auto dc = loplugin::DeclCheck(cxxRecordDecl); - if (dc.Class("SvXMLImportContext")) - { - rbFoundImportContext = true; - return false; - } if (dc.Class("XFastContextHandler")) return false; for (auto it = cxxRecordDecl->method_begin(); it != cxxRecordDecl->method_end(); ++it) { auto i = *it; if (i->getIdentifier() && i->getName() == "startFastElement") - { - // i->dump(); return true; - } } return false; } + bool XmlImport::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl) { auto beginLoc = compat::getBeginLoc(methodDecl); if (!beginLoc.isValid() || ignoreLocation(beginLoc)) return true; - auto cxxRecordDecl = methodDecl->getParent(); - if (!cxxRecordDecl || !cxxRecordDecl->getIdentifier()) - return true; - auto className = cxxRecordDecl->getName(); - if (className == "OOXMLFactory") // writerfilter - return true; - if (className == "SvXMLLegacyToFastDocHandler" || className == "ImportDocumentHandler" - || className == "ExportDocumentHandler") // reportdesign - return true; - if (className == "XMLEmbeddedObjectExportFilter" || className == "XMLBasicExportFilter" - || className == "XMLTransformerBase" || className == "SvXMLMetaExport") // xmloff - return true; if (!methodDecl->getIdentifier()) return true; - if (!(methodDecl->getName() == "createFastChildContext" || methodDecl->getName() == "characters" - || methodDecl->getName() == "endFastElement")) + + auto cxxRecordDecl = methodDecl->getParent(); + if (!cxxRecordDecl || !cxxRecordDecl->getIdentifier()) return true; + if (loplugin::DeclCheck(cxxRecordDecl).Class("SvXMLImportContext")) return true; - bool foundImportContext = false; - if (containsStartFastElementMethod(cxxRecordDecl, foundImportContext)) - return true; + if (methodDecl->getName() == "createFastChildContext" || methodDecl->getName() == "characters" + || methodDecl->getName() == "endFastElement") + { + auto className = cxxRecordDecl->getName(); + if (className == "OOXMLFactory") // writerfilter + return true; + if (className == "SvXMLLegacyToFastDocHandler" || className == "ImportDocumentHandler" + || className == "ExportDocumentHandler") // reportdesign + return true; + if (className == "XMLEmbeddedObjectExportFilter" || className == "XMLBasicExportFilter" + || className == "XMLTransformerBase" || className == "SvXMLMetaExport") // xmloff + return true; - bool foundStartFastElement = false; - CXXBasePaths aPaths; - cxxRecordDecl->lookupInBases( - [&](const CXXBaseSpecifier* Specifier, CXXBasePath & /*Path*/) -> bool { - if (!Specifier->getType().getTypePtr()) - return false; - const CXXRecordDecl* baseCXXRecordDecl = Specifier->getType()->getAsCXXRecordDecl(); - if (!baseCXXRecordDecl) + if (containsStartFastElementMethod(cxxRecordDecl)) + return true; + + bool foundStartFastElement = false; + bool foundImportContext = false; + + CXXBasePaths aPaths; + cxxRecordDecl->lookupInBases( + [&](const CXXBaseSpecifier* Specifier, CXXBasePath & /*Path*/) -> bool { + if (!Specifier->getType().getTypePtr()) + return false; + const CXXRecordDecl* baseCXXRecordDecl = Specifier->getType()->getAsCXXRecordDecl(); + if (!baseCXXRecordDecl) + return false; + if (baseCXXRecordDecl->isInvalidDecl()) + return false; + if (loplugin::DeclCheck(baseCXXRecordDecl).Class("SvXMLImportContext")) + foundImportContext |= true; + else + foundStartFastElement |= containsStartFastElementMethod(baseCXXRecordDecl); return false; - if (baseCXXRecordDecl->isInvalidDecl()) + }, + aPaths); + + if (foundImportContext && !foundStartFastElement) + report(DiagnosticsEngine::Warning, "must override startFastElement too", + compat::getBeginLoc(methodDecl)) + << methodDecl->getSourceRange(); + } + else if (methodDecl->getName() == "StartElement" || methodDecl->getName() == "EndElement" + || methodDecl->getName() == "Characters") + { + if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLAxisContext")) + return true; + if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLChartContext")) + return true; + if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLParagraphContext")) + return true; + if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLLegendContext")) + return true; + if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLPropertyMappingContext")) + return true; + + bool foundImportContext = false; + CXXBasePaths aPaths; + cxxRecordDecl->lookupInBases( + [&](const CXXBaseSpecifier* Specifier, CXXBasePath & /*Path*/) -> bool { + if (!Specifier->getType().getTypePtr()) + return false; + const CXXRecordDecl* baseCXXRecordDecl = Specifier->getType()->getAsCXXRecordDecl(); + if (!baseCXXRecordDecl) + return false; + if (baseCXXRecordDecl->isInvalidDecl()) + return false; + if (loplugin::DeclCheck(baseCXXRecordDecl).Class("SvXMLImportContext")) + foundImportContext |= true; return false; - foundStartFastElement - |= containsStartFastElementMethod(baseCXXRecordDecl, foundImportContext); - return false; - }, - aPaths); + }, + aPaths); - if (foundStartFastElement || !foundImportContext) - return true; + if (!foundImportContext) + return true; + + bool foundConstructor = false; + for (auto it = cxxRecordDecl->ctor_begin(); it != cxxRecordDecl->ctor_end(); ++it) + { + const CXXConstructorDecl* ctor = *it; + bool foundInt16 = false; + for (auto paramIt = ctor->param_begin(); paramIt != ctor->param_end(); ++paramIt) + { + const ParmVarDecl* pvd = *paramIt; + auto tc = loplugin::TypeCheck(pvd->getType()); + if (tc.Typedef("sal_uInt16")) + foundInt16 = true; + else if (tc.LvalueReference().Const().Class("OUString") && foundInt16) + foundConstructor = true; + else + foundInt16 = false; + if (tc.LvalueReference().Const().Class("OUString") + && pvd->getName() == "rLocalName") + foundConstructor = true; + } + } + + if (!foundConstructor) + report(DiagnosticsEngine::Warning, + "overrides startElement, but looks like a fastparser context class, no " + "constructor that takes slowparser args", + compat::getBeginLoc(methodDecl)) + << methodDecl->getSourceRange(); + } - report(DiagnosticsEngine::Warning, "must override startFastElement too", - compat::getBeginLoc(methodDecl)) - << methodDecl->getSourceRange(); return true; } -- cgit