From 8c5ffecf1dbd3f93128910433da11d5315661680 Mon Sep 17 00:00:00 2001 From: Noel Date: Fri, 23 Oct 2020 15:12:22 +0200 Subject: make SvXMLImport capable of mixing fast- and slow- contexts adhoc so I can convert even *ImportContext subclasses in the middle of a context stack, and thus break the cyclic dependency nature of the writer import. and adjust the xmlimport loplugin for the new rules. As a consequence of the loplugin:xmlimport's checking, we remove a bunch of now unnecessary overrides of startFastElement. Change-Id: I97464522ede8ec5e345e928cdafa4b18364b1b80 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104730 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/test/xmlimport.cxx | 155 ++++++++++++++--- compilerplugins/clang/xmlimport.cxx | 289 +++++++++++++++++++------------ 2 files changed, 308 insertions(+), 136 deletions(-) (limited to 'compilerplugins/clang') diff --git a/compilerplugins/clang/test/xmlimport.cxx b/compilerplugins/clang/test/xmlimport.cxx index 36230ffdb2d8..98dba400b19e 100644 --- a/compilerplugins/clang/test/xmlimport.cxx +++ b/compilerplugins/clang/test/xmlimport.cxx @@ -11,78 +11,185 @@ // Cannot include this, makes clang crash //#include "xmloff/xmlimp.hxx" +// Cannot include this, cannot be found +//#include #include +#include namespace com::sun::star::xml::sax { class XAttributeList; +class XFastContextHandler; } +class SvXMLImportContext; +typedef rtl::Reference SvXMLImportContextRef; class SvXMLImportContext { public: virtual ~SvXMLImportContext() {} - virtual void createFastChildContext() {} virtual void startFastElement() {} virtual void endFastElement() {} + virtual void characters(const OUString&) {} + virtual css::uno::Reference createFastChildContext() + { + return nullptr; + } + virtual css::uno::Reference createUnknownChildContext() + { + return nullptr; + } virtual void StartElement(const css::uno::Reference&) {} virtual void EndElement() {} virtual void Characters(const OUString&) {} + virtual SvXMLImportContextRef CreateChildContext() { return nullptr; } + + void acquire(); + void release(); + + void xxx(); // just here to avoid triggering a warning I don't want to check for }; class Test1 : public SvXMLImportContext { public: - // expected-error@+1 {{must override startFastElement too [loplugin:xmlimport]}} - virtual void createFastChildContext() override; + // expected-error@+1 {{cannot override both startFastElement and StartElement [loplugin:xmlimport]}} + virtual void startFastElement() override { xxx(); } + // expected-error@+1 {{cannot override both startFastElement and StartElement [loplugin:xmlimport]}} + virtual void StartElement(const css::uno::Reference&) override + { + xxx(); + } }; class Test2 : public SvXMLImportContext { public: - // no warning expected - virtual void createFastChildContext() override; - virtual void startFastElement() override {} + // expected-error@+1 {{cannot override both endFastElement and EndElement [loplugin:xmlimport]}} + virtual void endFastElement() override { xxx(); } + // expected-error@+1 {{cannot override both endFastElement and EndElement [loplugin:xmlimport]}} + virtual void EndElement() override { xxx(); } }; -class Test3 : public Test2 +class Test3 : public SvXMLImportContext { public: - // no warning expected - virtual void createFastChildContext() override; + // expected-error@+1 {{cannot override both characters and Characters [loplugin:xmlimport]}} + virtual void Characters(const OUString&) override { xxx(); } + // expected-error@+1 {{cannot override both characters and Characters [loplugin:xmlimport]}} + virtual void characters(const OUString&) override { xxx(); } }; -class Test4 : public SvXMLImportContext +class Test7 : public SvXMLImportContext { public: - // expected-error@+1 {{must override startFastElement too [loplugin:xmlimport]}} - virtual void endFastElement() override; + virtual void startFastElement() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::startFastElement(); + } + virtual void endFastElement() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::endFastElement(); + } + virtual void characters(const OUString& rChars) override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::characters(rChars); + } + virtual css::uno::Reference + createFastChildContext() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + return SvXMLImportContext::createFastChildContext(); + } + virtual css::uno::Reference + createUnknownChildContext() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + return SvXMLImportContext::createUnknownChildContext(); + } }; -class Test5 : public SvXMLImportContext +class Test8 : 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; + StartElement(const css::uno::Reference& xAttrList) override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::StartElement(xAttrList); + } + virtual void EndElement() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::EndElement(); + } + virtual void Characters(const OUString& rChars) override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + SvXMLImportContext::Characters(rChars); + } + virtual SvXMLImportContextRef CreateChildContext() override + { + // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}} + return SvXMLImportContext::CreateChildContext(); + } }; // no warning expected -class Test6 : public SvXMLImportContext +class Test9a : public SvXMLImportContext +{ +public: + virtual void StartElement(const css::uno::Reference&) override + { + xxx(); + } +}; +class Test9b : public Test9a { 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; + StartElement(const css::uno::Reference& xAttrList) override + { + Test9a::StartElement(xAttrList); + } +}; + +class Test10a : public SvXMLImportContext +{ +public: + // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}} + virtual void startFastElement() override {} + // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}} + virtual void endFastElement() override {} + // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}} + virtual void characters(const OUString&) override {} + // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}} + virtual css::uno::Reference + createFastChildContext() override + { + return nullptr; + } + // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}} + virtual css::uno::Reference + createUnknownChildContext() override + { + return nullptr; + } +}; +// no warning expected +class Test10b : public SvXMLImportContext +{ +public: + virtual void StartElement(const css::uno::Reference&) override {} + virtual void EndElement() override {} + virtual void Characters(const OUString&) override {} + virtual SvXMLImportContextRef CreateChildContext() override { return nullptr; } }; /* 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 d6b1aa78e325..72645564a5d1 100644 --- a/compilerplugins/clang/xmlimport.cxx +++ b/compilerplugins/clang/xmlimport.cxx @@ -14,6 +14,7 @@ #include "plugin.hxx" #include "check.hxx" #include +#include #include "clang/AST/CXXInheritance.h" /* @@ -36,8 +37,23 @@ public: bool preRun() override { - // std::string fn(handler.getMainFileName()); - // loplugin::normalizeDotDotInFilePath(fn); + StringRef fn(handler.getMainFileName()); + if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/core/xmlictxt.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/core/xmlimp.cxx")) + return false; + // These are mostly classes delegating calls to other classes + if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/text/XMLTextFrameContext.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/draw/ximpshap.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/table/XMLTableImport.cxx")) + return false; + if (loplugin::isSamePathname(fn, + SRCDIR "/sc/source/filter/xml/XMLTrackedChangesContext.cxx")) + return false; + if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/xml/xmlannoi.cxx")) + return false; return true; } @@ -50,21 +66,16 @@ public: } bool VisitCXXMethodDecl(const CXXMethodDecl*); -}; + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr*); -static bool containsStartFastElementMethod(const CXXRecordDecl* cxxRecordDecl) -{ - auto dc = loplugin::DeclCheck(cxxRecordDecl); - 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") - return true; - } - return false; -} +private: + std::unordered_map startFastElementSet; + std::unordered_map StartElementSet; + std::unordered_map endFastElementSet; + std::unordered_map EndElementSet; + std::unordered_map charactersSet; + std::unordered_map CharactersSet; +}; bool XmlImport::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl) { @@ -82,111 +93,165 @@ bool XmlImport::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl) if (loplugin::DeclCheck(cxxRecordDecl).Class("SvXMLImportContext")) return true; - if (methodDecl->getName() == "createFastChildContext" || methodDecl->getName() == "characters" - || methodDecl->getName() == "endFastElement") + if (!loplugin::isDerivedFrom(cxxRecordDecl, [](Decl const* decl) -> bool { + auto const dc = loplugin::DeclCheck(decl); + return bool(dc.ClassOrStruct("SvXMLImportContext").GlobalNamespace()); + })) + return true; + + auto name = methodDecl->getName(); + if (name == "startFastElement") + startFastElementSet.insert({ cxxRecordDecl, methodDecl }); + else if (name == "StartElement") + StartElementSet.insert({ cxxRecordDecl, methodDecl }); + else if (name == "endFastElement") + endFastElementSet.insert({ cxxRecordDecl, methodDecl }); + else if (name == "EndElement") + EndElementSet.insert({ cxxRecordDecl, methodDecl }); + else if (name == "characters") { - 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 (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; - }, - aPaths); - - if (foundImportContext && !foundStartFastElement) - report(DiagnosticsEngine::Warning, "must override startFastElement too", - compat::getBeginLoc(methodDecl)) - << methodDecl->getSourceRange(); + if (methodDecl->getNumParams() == 1) + charactersSet.insert({ cxxRecordDecl, methodDecl }); } - else if (methodDecl->getName() == "StartElement" || methodDecl->getName() == "EndElement" - || methodDecl->getName() == "Characters") + else if (name == "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; - }, - aPaths); - - if (!foundImportContext) - return true; - - bool foundConstructor = false; - for (auto it = cxxRecordDecl->ctor_begin(); it != cxxRecordDecl->ctor_end(); ++it) + if (methodDecl->getNumParams() == 1) + CharactersSet.insert({ cxxRecordDecl, methodDecl }); + } + + { + auto it1 = endFastElementSet.find(cxxRecordDecl); + auto it2 = EndElementSet.find(cxxRecordDecl); + if (it1 != endFastElementSet.end() && it2 != EndElementSet.end()) { - 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; - } + auto methodDecl1 = it1->second; + report(DiagnosticsEngine::Warning, "cannot override both endFastElement and EndElement", + compat::getBeginLoc(methodDecl1)) + << methodDecl1->getSourceRange(); + auto methodDecl2 = it2->second; + report(DiagnosticsEngine::Warning, "cannot override both endFastElement and EndElement", + compat::getBeginLoc(methodDecl2)) + << methodDecl2->getSourceRange(); } + } - if (!foundConstructor) + { + auto it1 = startFastElementSet.find(cxxRecordDecl); + auto it2 = StartElementSet.find(cxxRecordDecl); + if (it1 != startFastElementSet.end() && it2 != StartElementSet.end()) + { + auto methodDecl1 = it1->second; + report(DiagnosticsEngine::Warning, + "cannot override both startFastElement and StartElement", + compat::getBeginLoc(methodDecl1)) + << methodDecl1->getSourceRange(); + auto methodDecl2 = it2->second; report(DiagnosticsEngine::Warning, - "overrides startElement, but looks like a fastparser context class, no " - "constructor that takes slowparser args", - compat::getBeginLoc(methodDecl)) - << methodDecl->getSourceRange(); + "cannot override both startFastElement and StartElement", + compat::getBeginLoc(methodDecl2)) + << methodDecl2->getSourceRange(); + } + } + { + auto it1 = charactersSet.find(cxxRecordDecl); + auto it2 = CharactersSet.find(cxxRecordDecl); + if (it1 != charactersSet.end() && it2 != CharactersSet.end()) + { + auto methodDecl1 = it1->second; + report(DiagnosticsEngine::Warning, "cannot override both characters and Characters", + compat::getBeginLoc(methodDecl1)) + << methodDecl1->getSourceRange(); + auto methodDecl2 = it2->second; + report(DiagnosticsEngine::Warning, "cannot override both characters and Characters", + compat::getBeginLoc(methodDecl2)) + << methodDecl2->getSourceRange(); + } } + auto checkEmpty = [&]() { + if (!methodDecl->isThisDeclarationADefinition()) + return; + auto compoundStmt = dyn_cast_or_null(methodDecl->getBody()); + if (compoundStmt == nullptr || compoundStmt->size() > 0) + return; + report(DiagnosticsEngine::Warning, "empty, should be removed", + compat::getBeginLoc(methodDecl)) + << methodDecl->getSourceRange(); + auto canonicalDecl = methodDecl->getCanonicalDecl(); + if (canonicalDecl != methodDecl) + report(DiagnosticsEngine::Note, "definition here", compat::getBeginLoc(canonicalDecl)) + << canonicalDecl->getSourceRange(); + }; + auto checkOnlyReturn = [&]() { + if (!methodDecl->isThisDeclarationADefinition()) + return; + auto compoundStmt = dyn_cast_or_null(methodDecl->getBody()); + if (compoundStmt == nullptr || compoundStmt->size() > 1) + return; + auto returnStmt = dyn_cast_or_null(*compoundStmt->body_begin()); + if (!returnStmt) + return; + auto cxxConstructExpr + = dyn_cast_or_null(returnStmt->getRetValue()->IgnoreImplicit()); + if (!cxxConstructExpr) + return; + if (cxxConstructExpr->getNumArgs() != 1) + return; + if (!isa(cxxConstructExpr->getArg(0)->IgnoreImplicit())) + return; + report(DiagnosticsEngine::Warning, "empty, should be removed", + compat::getBeginLoc(methodDecl)) + << methodDecl->getSourceRange(); + auto canonicalDecl = methodDecl->getCanonicalDecl(); + if (canonicalDecl != methodDecl) + report(DiagnosticsEngine::Note, "definition here", compat::getBeginLoc(canonicalDecl)) + << canonicalDecl->getSourceRange(); + }; + + if (name == "startFastElement") + checkEmpty(); + else if (name == "endFastElement") + checkEmpty(); + else if (name == "characters") + checkEmpty(); + else if (name == "createFastChildContext") + checkOnlyReturn(); + else if (name == "createUnknownChildContext") + checkOnlyReturn(); + + return true; +} + +bool XmlImport::VisitCXXMemberCallExpr(const CXXMemberCallExpr* callExpr) +{ + auto beginLoc = compat::getBeginLoc(callExpr); + if (!beginLoc.isValid() || ignoreLocation(callExpr)) + return true; + + CXXMethodDecl* methodDecl = callExpr->getMethodDecl(); + if (!methodDecl || !methodDecl->getIdentifier()) + return true; + + auto cxxRecordDecl = methodDecl->getParent(); + if (!cxxRecordDecl || !cxxRecordDecl->getIdentifier()) + return true; + + if (!loplugin::DeclCheck(cxxRecordDecl).Class("SvXMLImportContext")) + return true; + + auto name = methodDecl->getName(); + if (name == "startFastElement" || name == "characters" || name == "endFastElement" + || name == "createFastChildContext" || name == "createUnknownChildContext" + || name == "StartElement" || name == "EndElement" || name == "Characters" + || name == "CreateChildContext") + { + /** + * Calling this superclass method from a subclass method will mess with the fallback logic in the superclass. + */ + report(DiagnosticsEngine::Warning, "don't call this superclass method", + compat::getBeginLoc(callExpr)) + << callExpr->getSourceRange(); + } return true; } -- cgit