update loplugin returnconstant

to find more stuff, and return less false positives

Change-Id: I24cacbb825c1f7484fd568230051b1a57dbc663f
Reviewed-on: https://gerrit.libreoffice.org/59137
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

1 files changed, 48 insertions, 486 deletions

diff --git a/compilerplugins/clang/returnconstant.cxx b/compilerplugins/clang/returnconstant.cxx
index eff3495191fd..a11cff3d11de 100644
--- a/compilerplugins/clang/returnconstant.cxx
+++ b/compilerplugins/clang/returnconstant.cxx
@@ -9,6 +9,8 @@
 
 #include "plugin.hxx"
 #include "check.hxx"
+#include "compat.hxx"
+#include "functionaddress.hxx"
 #include <iostream>
 #include <set>
 #include <string>
@@ -21,33 +23,40 @@
 */
 namespace
 {
-class ReturnConstant : public RecursiveASTVisitor<ReturnConstant>, public loplugin::Plugin
+class ReturnConstant : public loplugin::FunctionAddress<ReturnConstant>
 {
 public:
     explicit ReturnConstant(loplugin::InstantiationData const& data)
-        : Plugin(data)
+        : loplugin::FunctionAddress<ReturnConstant>(data)
     {
     }
 
     void run() override
     {
-        // these files crash clang-3.5 somewhere in the isEvaluatable/EvaluateAsXXX stuff
-        /*        FileID mainFileID = compiler.getSourceManager().getMainFileID();
-        if (strstr(compiler.getSourceManager().getFileEntryForID(mainFileID)->getName(), "bootstrapfixture.cxx") != 0) {
-            return;
-        }
-        if (strstr(compiler.getSourceManager().getFileEntryForID(mainFileID)->getName(), "gtk3gtkinst.cxx") != 0) {
-            return;
-        }*/
-
         TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+        for (auto& pair : problemFunctions)
+        {
+            auto functionDecl = pair.first;
+            auto canonicalDecl = functionDecl->getCanonicalDecl();
+            if (getFunctionsWithAddressTaken().find(canonicalDecl)
+                != getFunctionsWithAddressTaken().end())
+                continue;
+            report(DiagnosticsEngine::Warning,
+                   "Method only returns a single constant value %0, does it make sense?",
+                   compat::getBeginLoc(functionDecl))
+                << pair.second << functionDecl->getSourceRange();
+            if (functionDecl != functionDecl->getCanonicalDecl())
+                report(DiagnosticsEngine::Note, "decl here",
+                       compat::getBeginLoc(functionDecl->getCanonicalDecl()))
+                    << functionDecl->getCanonicalDecl()->getSourceRange();
+        }
     }
 
     bool TraverseCXXMethodDecl(CXXMethodDecl*);
     bool VisitReturnStmt(ReturnStmt const*);
 
 private:
-    StringRef getFilename(FunctionDecl const* functionDecl);
     std::string getExprValue(Expr const* arg);
 
     struct Context
@@ -56,174 +65,43 @@ private:
         std::set<std::string> values;
     };
     std::vector<Context> m_functionStack;
+    std::vector<std::pair<FunctionDecl const*, std::string>> problemFunctions;
 };
 
-StringRef ReturnConstant::getFilename(FunctionDecl const* functionDecl)
-{
-    SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(
-        functionDecl->getCanonicalDecl()->getNameInfo().getLoc());
-    StringRef name{ compiler.getSourceManager().getFilename(spellingLocation) };
-    return name;
-}
-
-static bool startsWith(std::string const& rStr, char const* pSubStr)
-{
-    return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
-}
-
 bool ReturnConstant::TraverseCXXMethodDecl(CXXMethodDecl* functionDecl)
 {
     if (ignoreLocation(functionDecl))
-    {
         return true;
-    }
+    if (isInUnoIncludeFile(functionDecl))
+        return true;
+
     if (!functionDecl->hasBody())
-    {
         return true;
-    }
     if (!functionDecl->isThisDeclarationADefinition())
-    {
         return true;
-    }
-    // stuff declared extern-C is almost always used as a some kind of callback
-    if (functionDecl->isExternC())
-    {
-        return true;
-    }
     if (functionDecl->isConstexpr())
-    {
-        return true;
-    }
-    if (functionDecl->isMain())
-    {
-        return true;
-    }
-
-    StringRef aFileName = getFilename(functionDecl);
-
-    // various tests in here are empty stubs under Linux
-    if (aFileName.startswith(SRCDIR "/sal/qa/"))
-    {
-        return true;
-    }
-    // lots of empty stuff here where it looks like someone is still going to "fill in the blanks"
-    if (aFileName.startswith(SRCDIR "/basegfx/test/"))
-    {
-        return true;
-    }
-    // some stuff is just stubs under Linux, although this appears to be a SOLARIS-specific hack, so it
-    // should probably not even be compiling under Linux.
-    if (aFileName == SRCDIR "/setup_native/scripts/source/getuid.c")
-    {
-        return true;
-    }
-    // bridges has some weird stuff in it....
-    if (aFileName.startswith(SRCDIR "/bridges/"))
-    {
-        return true;
-    }
-    // dummy implementation of DDE, since it is only active on Windows
-    if (aFileName == SRCDIR "/svl/unx/source/svdde/ddedummy.cxx"
-        || aFileName == SRCDIR "/include/svl/svdde.hxx")
-    {
-        return true;
-    }
-    // fancy templates at work here
-    if (aFileName == SRCDIR "/vcl/source/gdi/bmpfast.cxx")
-    {
-        return true;
-    }
-    // bunch of stuff used as callbacks here
-    if (aFileName == SRCDIR "/vcl/generic/glyphs/gcach_layout.cxx")
-    {
-        return true;
-    }
-    // salplug runtime-loading mechanism at work
-    if (aFileName == SRCDIR "/vcl/inc/salinst.hxx")
-    {
-        return true;
-    }
-    // lots of callbacks here
-    if (aFileName == SRCDIR "/extensions/source/plugin/unx/npnapi.cxx")
-    {
-        return true;
-    }
-    // template magic
-    if (aFileName == SRCDIR "/filter/source/svg/svgreader.cxx")
-    {
-        return true;
-    }
-    // vcl/unx/gtk3 re-using vcl/unx/gtk:
-    if (aFileName.find("/../../gtk/") != std::string::npos)
-    {
-        return true;
-    }
-    // used by code generated by python
-    if (aFileName == SRCDIR "/writerfilter/source/ooxml/OOXMLFastContextHandler.hxx")
-    {
-        return true;
-    }
-    // this test just test the include of some headers
-    if (aFileName == SRCDIR "/officecfg/qa/cppheader.cxx")
-    {
-        return true;
-    }
-    // just ignore this for now, people furiously hacking in there
-    if (startsWith(aFileName, SRCDIR "/libreofficekit"))
-    {
-        return true;
-    }
-    // templates
-    if (startsWith(aFileName, SRCDIR "/store"))
-        return true;
-
-    // not worth it
-    if (startsWith(aFileName, SRCDIR "/registry"))
         return true;
-    if (startsWith(aFileName, SRCDIR "/idlc"))
+    if (functionDecl->getReturnType()->isVoidType())
         return true;
-    if (startsWith(aFileName, SRCDIR "/include/unotools"))
+    if (functionDecl->isVirtual())
         return true;
-    if (startsWith(aFileName, SRCDIR "/xmlreader/"))
+    // static with inline body will be optimised at compile-time to a constant anyway
+    if (functionDecl->isStatic()
+        && (functionDecl->hasInlineBody() || functionDecl->isInlineSpecified()))
         return true;
-    if (startsWith(aFileName, SRCDIR "/include/xmlreader/"))
-        return true;
-    if (startsWith(aFileName, SRCDIR "/jvmfwk"))
+    // this catches some stuff in templates
+    if (functionDecl->hasAttr<OverrideAttr>())
         return true;
 
-    const CXXMethodDecl* pCXXMethodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
-    if (pCXXMethodDecl)
-    {
-        if (pCXXMethodDecl->isVirtual())
-        {
-            return true;
-        }
-        // static with inline body will be optimised at compile-time to a constant anyway
-        if (pCXXMethodDecl->isStatic()
-            && (pCXXMethodDecl->hasInlineBody() || pCXXMethodDecl->isInlineSpecified()))
-        {
-            return true;
-        }
-        // this catches some stuff in templates
-        if (functionDecl->hasAttr<OverrideAttr>())
-        {
-            return true;
-        }
-    }
-    // a free function with an inline body will be optimised at compile-time to a constant anyway
-    if (!pCXXMethodDecl && functionDecl->isInlineSpecified())
-    {
+    // include/unotools/localedatawrapper.hxx
+    if (functionDecl->getIdentifier() && functionDecl->getName() == "getCurrZeroChar")
         return true;
-    }
-    if (isa<CXXConstructorDecl>(functionDecl) || isa<CXXDestructorDecl>(functionDecl)
-        || isa<CXXConversionDecl>(functionDecl))
-    {
+    // sc/inc/stlalgorithm.hxx
+    if (loplugin::DeclCheck(functionDecl->getParent())
+            .Class("AlignedAllocator")
+            .Namespace("sc")
+            .GlobalNamespace())
         return true;
-    }
-    if (isInUnoIncludeFile(functionDecl))
-    {
-        return true;
-    }
 
     switch (functionDecl->getOverloadedOperator())
     {
@@ -235,333 +113,24 @@ bool ReturnConstant::TraverseCXXMethodDecl(CXXMethodDecl* functionDecl)
             break;
     }
 
-    std::string aFunctionName = functionDecl->getQualifiedNameAsString();
-
-    // something to do with dynamic loading in sal/textenc/textenc.cxx
-    if (aFunctionName == "thisModule")
-    {
-        return true;
-    }
-    // an empty stub under certain conditions, sal/osl/unx/thread.cxx
-    if (aFunctionName == "osl_thread_priority_init_Impl")
-    {
-        return true;
-    }
-    // a pointer to this function is taken and passed to an underlying API, shell/source/unix/sysshell/recently_used_file_handler.cxx
-    if (aFunctionName == "(anonymous namespace)::recently_used_item::set_nothing")
-    {
-        return true;
-    }
-    // a pointer to this function is taken and passed to an underlying API, cppu/source/uno/lbenv.cxx
-    if (aFunctionName == "defenv_dispose")
-    {
-        return true;
-    }
-    // a pointer to this function is taken and passed to an underlying API, cppuhelper/source/exc_thrower.cxx
-    if (aFunctionName == "ExceptionThrower_acquire_release_nop")
-    {
-        return true;
-    }
-    // different hook function is called on different platforms, /vcl/source/app/svmainhook.cxx
-    if (aFunctionName == "ImplSVMainHook")
-    {
-        return true;
-    }
-    // used as a callback, /vcl/source/filter/jpeg/JpegReader.cxx
-    if (aFunctionName == "term_source")
-    {
-        return true;
-    }
-    // only valid for windows, extensions/source/update/check/updatecheck.cxx
-    if (aFunctionName == "(anonymous namespace)::UpdateCheckThread::hasInternetConnection")
-    {
-        return true;
-    }
-    // used as callback, extensions/source/plugin/unx/npwrap.cxx
-    if (aFunctionName == "plugin_x_error_handler" || aFunctionName == "noClosure")
-    {
-        return true;
-    }
-    // used as callback, sax/source/expatwrap/sax_expat.cxx
-    if (aFunctionName == "(anonymous namespace)::SaxExpatParser_Impl::callbackUnknownEncoding")
-    {
-        return true;
-    }
-    // used as callback, i18npool/source/textconversion/textconversion.cxx
-    if (aFunctionName == "com::sun::star::i18n::nullFunc")
-    {
-        return true;
-    }
-    // used as callback, xmloff/source/text/txtparae.cxx
-    if (aFunctionName == "(anonymous namespace)::lcl_TextContentsUnfiltered")
-    {
-        return true;
-    }
-    // template magic, include/canvas/verifyinput.hxx
-    if (aFunctionName == "canvas::tools::verifyInput")
-    {
-        return true;
-    }
-    // template magic, cppcanvas/source/mtfrenderer/implrenderer.cxx
-    if (aFunctionName == "cppcanvas::internal::(anonymous namespace)::AreaQuery::result")
-    {
-        return true;
-    }
-    // callback, drawinglayer/source/dumper/XShapeDumper.
-    if (aFunctionName == "(anonymous namespace)::closeCallback")
-    {
-        return true;
-    }
-    // callback, basic/source/runtime/runtime.cxx
-    if (aFunctionName == "SbiRuntime::StepNOP")
-    {
-        return true;
-    }
-    // DLL stuff, only used on windows, basic/source/runtime/dllmgr.hxx
-    if (aFunctionName == "SbiDllMgr::FreeDll")
-    {
-        return true;
-    }
-    // only used on Windows, basic/source/sbx/sbxdec.cxx
-    if (aFunctionName == "SbxDecimal::neg" || aFunctionName == "SbxDecimal::isZero")
-    {
-        return true;
-    }
-    // used as a callback, include/sfx2/shell.hxx
-    if (aFunctionName == "SfxShell::EmptyExecStub" || aFunctionName == "SfxShell::EmptyStateStub"
-        || aFunctionName == "SfxShell::VerbState")
-    {
-        return true;
-    }
-    // SFX_IMPL_POS_CHILDWINDOW_WITHID macro
-    if (aFunctionName.find("GetChildWindowId") != std::string::npos)
-    {
-        return true;
-    }
-    // SFX_IMPL_SUPERCLASS_INTERFACE macro
-    if (aFunctionName.find("InitInterface_Impl") != std::string::npos)
-    {
-        return true;
-    }
-    // callback, vcl/unx/generic/app/sm.cxx
-    if (aFunctionName == "IgnoreIceIOErrors" || aFunctionName == "IgnoreIceErrors")
-    {
-        return true;
-    }
-    // callback, vcl/unx/gtk/a11y/atkcomponent.cxx
-    if (aFunctionName == "component_wrapper_get_mdi_zorder")
-    {
-        return true;
-    }
-    // callback, vcl/unx/gtk/a11y/atkaction.cxx
-    if (aFunctionName == "action_wrapper_set_description")
-    {
-        return true;
-    }
-    // callback, vcl/unx/gtk/a11y/atkutil.cxx
-    if (aFunctionName == "ooo_atk_util_get_toolkit_version"
-        || aFunctionName == "ooo_atk_util_get_toolkit_name")
-    {
-        return true;
-    }
-    // callback, vcl/unx/gtk/a11y/atktextattributes.cxx
-    if (aFunctionName == "InvalidValue")
-    {
-        return true;
-    }
-    // callback, vcl/unx/gtk/a11y/atktable.cxx
-    if (aFunctionName == "table_wrapper_set_summary"
-        || aFunctionName == "table_wrapper_set_row_header"
-        || aFunctionName == "table_wrapper_set_row_description"
-        || aFunctionName == "table_wrapper_set_column_header"
-        || aFunctionName == "table_wrapper_set_column_description"
-        || aFunctionName == "table_wrapper_set_caption")
-    {
-        return true;
-    }
-    // callbacks, vcl/unx/gtk/window/gtksalframe.cxx
-    if (startsWith(aFunctionName, "GtkSalFrame::IMHandler::signal"))
-    {
-        return true;
-    }
-    // callbacks, vcl/unx/gtk/window/glomenu.cxx
-    if (startsWith(aFunctionName, "g_lo_menu_is_mutable"))
-    {
-        return true;
-    }
-    // only contains code for certain versions of GTK, /vcl/unx/gtk/window/gtksalframe.cx
-    if (aFunctionName == "GtkSalFrame::AllocateFrame")
-    {
-        return true;
-    }
-    // only valid for Windows, embeddedobj/source/msole/olemisc.cxx
-    if (aFunctionName == "OleEmbeddedObject::GetRidOfComponent")
-    {
-        return true;
-    }
-    // callback, svx/source/accessibility/ShapeTypeHandler.cxx
-    if (aFunctionName == "accessibility::CreateEmptyShapeReference")
-    {
-        return true;
-    }
-    //  chart2/source/view/main/AbstractShapeFactory.cxx
-    if (aFunctionName == "chart::(anonymous namespace)::thisModule")
-    {
-        return true;
-    }
-    //  chart2/source/tools/InternalData.cxx
-    if (aFunctionName == "chart::InternalData::dump")
-    {
-        return true;
-    }
-    //  hwpfilter/
-    if (aFunctionName == "debug" || aFunctionName == "token_debug")
-    {
-        return true;
-    }
-    //  callback, sdext/source/presenter/PresenterFrameworkObserver.cxx
-    if (aFunctionName == "sdext::presenter::PresenterFrameworkObserver::True")
-    {
-        return true;
-    }
-    // callback, sw/source/core/doc/tblrwcl.cxx
-    if (aFunctionName == "lcl_DelOtherBox")
-    {
-        return true;
-    }
-    // callback, sw/source/filter/ww8/ww8par.cxx
-    if (aFunctionName == "SwWW8ImplReader::Read_Majority")
-    {
-        return true;
-    }
-    // callback, sw/source/filter/ww8/ww8par5.cxx
-    if (aFunctionName == "SwWW8ImplReader::Read_F_Shape")
-    {
-        return true;
-    }
-    // called from SDI file, I don't know what that stuff is about, sd/source/ui/slidesorter/shell/SlideSorterViewShell.cx
-    if (aFunctionName == "sd::slidesorter::SlideSorterViewShell::ExecStatusBar"
-        || aFunctionName == "sd::OutlineViewShell::ExecStatusBar")
-    {
-        return true;
-    }
-    // only used in debug mode, sd/source/filter/ppt/pptinanimations.cxx
-    if (startsWith(aFunctionName, "ppt::AnimationImporter::dump"))
-    {
-        return true;
-    }
-    // only used in ENABLE_SDREMOTE_BLUETOOTH mode, sd/source/ui/dlg/tpoption.cx
-    if (aFunctionName == "SdTpOptionsMisc::SetImpressMode")
-    {
-        return true;
-    }
-    // template magic, sc/source/ui/docshell/datastream.cxx
-    if (startsWith(aFunctionName, "sc::(anonymous namespace)::CSVHandler::"))
-    {
-        return true;
-    }
-    // called from SDI file, I don't know what that stuff is about, sc/source/ui/view/cellsh4.cxx
-    if (aFunctionName == "ScCellShell::GetStateCursor")
-    {
-        return true;
-    }
-    // template magic, sc/source/filter/excel/xepivot.cxx
-    if (aFunctionName == "XclExpPivotCache::SaveXml")
-    {
-        return true;
-    }
-    // template magic, sc/source/filter/html/htmlpars.cxx
-    if (startsWith(aFunctionName, "(anonymous namespace)::CSSHandler::"))
-    {
-        return true;
-    }
-    // callbacks, sc/source/filter/oox/formulaparser.cxx
-    if (startsWith(aFunctionName, "oox::xls::BiffFormulaParserImpl::import"))
-    {
-        return true;
-    }
-    // template magic, sc/qa/unit/helper/csv_handler.hxx
-    if (startsWith(aFunctionName, "csv_handler::")
-        || startsWith(aFunctionName, "conditional_format_handler::"))
-    {
-        return true;
-    }
-    // template magic, slideshow/source/inc/listenercontainer.hxx
-    if (startsWith(aFunctionName, "slideshow::internal::EmptyBase::EmptyClearableGuard::"))
-    {
-        return true;
-    }
-    // callback, scripting/source/vbaevents/eventhelper.cxx
-    if (aFunctionName == "ApproveAll")
-    {
-        return true;
-    }
-    // only on WNT, basic/qa/cppunit/test_vba.cx
-    if (aFunctionName == "(anonymous namespace)::VBATest::testMiscOLEStuff")
-    {
-        return true;
-    }
-    // GtkSalFrame::TriggerPaintEvent() is only compiled under certain versions of GTK
-    if (aFunctionName == "GtkSalFrame::TriggerPaintEvent")
-    {
-        return true;
-    }
-    if (aFunctionName == "SwVectorModifyBase::dumpAsXml")
-    {
-        return true;
-    }
-    // vcl/unx/gtk3 re-using vcl/unx/gtk:
-    if (aFunctionName == "DeInitAtkBridge" || aFunctionName == "GtkData::initNWF"
-        || aFunctionName == "GtkSalFrame::EnsureAppMenuWatch" || aFunctionName == "InitAtkBridge")
-    {
-        return true;
-    }
-    if (aFunctionName == "sc::AlignedAllocator::operator!=")
-    {
+    // gtk signals and slots stuff
+    if (loplugin::TypeCheck(functionDecl->getReturnType()).Typedef("gboolean"))
         return true;
-    }
-    if (aFunctionName == "clipboard_owner_init")
-    {
-        return true;
-    }
-    // returns sizeof(struct) vcl/source/gdi/dibtools.cxx
-    if (aFunctionName == "getDIBV5HeaderSize")
-    {
-        return true;
-    }
-    // windows only
-    if (aFunctionName == "InitAccessBridge")
-    {
-        return true;
-    }
-    // callbacks
-    if (aFunctionName == "disabled_initSystray" || aFunctionName == "disabled_deInitSystray")
-    {
-        return true;
-    }
-    // behind a BREAKPAD option
-    if (aFunctionName == "desktop::(anonymous namespace)::crashReportInfoExists")
-    {
-        return true;
-    }
-    // LOK stuff
-    if (aFunctionName == "doc_getTileMode")
-    {
-        return true;
-    }
 
     // ignore LINK macro stuff
     std::string aImmediateMacro = "";
-    if (compiler.getSourceManager().isMacroBodyExpansion(compat::getBeginLoc(functionDecl)))
+    if (compiler.getSourceManager().isMacroBodyExpansion(compat::getBeginLoc(functionDecl))
+        || compiler.getSourceManager().isMacroArgExpansion(compat::getBeginLoc(functionDecl)))
     {
         StringRef name{ Lexer::getImmediateMacroName(compat::getBeginLoc(functionDecl),
                                                      compiler.getSourceManager(),
                                                      compiler.getLangOpts()) };
         aImmediateMacro = name;
-        if (name.startswith("IMPL_LINK_"))
-        {
+        if (name.find("IMPL_LINK") != StringRef::npos
+            || name.find("IMPL_STATIC_LINK") != StringRef::npos
+            || name.find("DECL_LINK") != StringRef::npos
+            || name.find("SFX_IMPL_POS_CHILDWINDOW_WITHID") != StringRef::npos)
             return true;
-        }
     }
 
     m_functionStack.emplace_back();
@@ -570,14 +139,7 @@ bool ReturnConstant::TraverseCXXMethodDecl(CXXMethodDecl* functionDecl)
     if (!rContext.ignore && rContext.values.size() == 1
         && rContext.values.find("unknown") == rContext.values.end())
     {
-        report(DiagnosticsEngine::Warning,
-               "Method only returns a single constant value %0, does it make sense?",
-               compat::getBeginLoc(functionDecl))
-            << *rContext.values.begin() << functionDecl->getSourceRange();
-        if (functionDecl != functionDecl->getCanonicalDecl())
-            report(DiagnosticsEngine::Note, "decl here",
-                   compat::getBeginLoc(functionDecl->getCanonicalDecl()))
-                << functionDecl->getCanonicalDecl()->getSourceRange();
+        problemFunctions.push_back({ functionDecl, *rContext.values.begin() });
     }
     m_functionStack.pop_back();
     return ret;