summaryrefslogtreecommitdiff
path: root/compilerplugins
diff options
context:
space:
mode:
authorNoel <noelgrandin@gmail.com>2020-10-23 15:12:22 +0200
committerNoel Grandin <noel.grandin@collabora.co.uk>2020-11-02 08:17:00 +0100
commit8c5ffecf1dbd3f93128910433da11d5315661680 (patch)
tree4263d40f2e6e118f42b7eed7bc40e92ad504036d /compilerplugins
parent0f3a8a972421aa440f4276b92463a481e5cd4267 (diff)
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 <noel.grandin@collabora.co.uk>
Diffstat (limited to 'compilerplugins')
-rw-r--r--compilerplugins/clang/test/xmlimport.cxx155
-rw-r--r--compilerplugins/clang/xmlimport.cxx289
2 files changed, 308 insertions, 136 deletions
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 <xmloff/xmlictxt.hxx>
#include <com/sun/star/uno/Reference.hxx>
+#include <rtl/ref.hxx>
namespace com::sun::star::xml::sax
{
class XAttributeList;
+class XFastContextHandler;
}
+class SvXMLImportContext;
+typedef rtl::Reference<SvXMLImportContext> SvXMLImportContextRef;
class SvXMLImportContext
{
public:
virtual ~SvXMLImportContext() {}
- virtual void createFastChildContext() {}
virtual void startFastElement() {}
virtual void endFastElement() {}
+ virtual void characters(const OUString&) {}
+ virtual css::uno::Reference<css::xml::sax::XFastContextHandler> createFastChildContext()
+ {
+ return nullptr;
+ }
+ virtual css::uno::Reference<css::xml::sax::XFastContextHandler> createUnknownChildContext()
+ {
+ return nullptr;
+ }
virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) {}
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<css::xml::sax::XAttributeList>&) 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<css::xml::sax::XFastContextHandler>
+ createFastChildContext() override
+ {
+ // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+ return SvXMLImportContext::createFastChildContext();
+ }
+ virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+ 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<css::xml::sax::XAttributeList>& 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<css::xml::sax::XAttributeList>& 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<css::xml::sax::XAttributeList>&) override
+ {
+ xxx();
+ }
+};
+class Test9b : public Test9a
{
public:
- Test6(sal_uInt16, const OUString&);
virtual void
- StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& xAttrList) override;
- virtual void EndElement() override;
- virtual void Characters(const OUString&) override;
+ StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& 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<css::xml::sax::XFastContextHandler>
+ createFastChildContext() override
+ {
+ return nullptr;
+ }
+ // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+ virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+ createUnknownChildContext() override
+ {
+ return nullptr;
+ }
+};
+// no warning expected
+class Test10b : public SvXMLImportContext
+{
+public:
+ virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) 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 <iostream>
+#include <unordered_map>
#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<const CXXRecordDecl*, const CXXMethodDecl*> startFastElementSet;
+ std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> StartElementSet;
+ std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> endFastElementSet;
+ std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> EndElementSet;
+ std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> charactersSet;
+ std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> 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<CompoundStmt>(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<CompoundStmt>(methodDecl->getBody());
+ if (compoundStmt == nullptr || compoundStmt->size() > 1)
+ return;
+ auto returnStmt = dyn_cast_or_null<ReturnStmt>(*compoundStmt->body_begin());
+ if (!returnStmt)
+ return;
+ auto cxxConstructExpr
+ = dyn_cast_or_null<CXXConstructExpr>(returnStmt->getRetValue()->IgnoreImplicit());
+ if (!cxxConstructExpr)
+ return;
+ if (cxxConstructExpr->getNumArgs() != 1)
+ return;
+ if (!isa<CXXNullPtrLiteralExpr>(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;
}