Extend loplugin:redundantinline to catch inline functions w/o external linkage

...where "inline" (in its meaning of "this function can be defined in multiple
translation units") thus doesn't make much sense.  (As discussed in
compilerplugins/clang/redundantinline.cxx, exempt such "static inline" functions
in include files for now.)

All the rewriting has been done automatically by the plugin, except for one
instance in sw/source/ui/frmdlg/column.cxx that used to involve an #if), plus
some subsequent solenv/clang-format/reformat-formatted-files.

Change-Id: Ib8b996b651aeafc03bbdc8890faa05ed50517224
Reviewed-on: https://gerrit.libreoffice.org/61573
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

3 files changed, 100 insertions, 38 deletions

diff --git a/compilerplugins/clang/redundantinline.cxx b/compilerplugins/clang/redundantinline.cxx
index 223f37ac98cb..30f970caf9d2 100644
--- a/compilerplugins/clang/redundantinline.cxx
+++ b/compilerplugins/clang/redundantinline.cxx
@@ -20,27 +20,24 @@ public:
     explicit RedundantInline(loplugin::InstantiationData const & data):
         FilteringRewritePlugin(data) {}
 
-    void run() override {
-        if (compiler.getLangOpts().CPlusPlus) {
-            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-        }
-    }
+    void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
 
     bool VisitFunctionDecl(FunctionDecl const * decl) {
-        if (ignoreLocation(decl) || !decl->isInlineSpecified()
-            || !(decl->doesThisDeclarationHaveABody()
-                 || decl->isExplicitlyDefaulted())
-            || !(decl->getLexicalDeclContext()->isRecord()
-                 || decl->isConstexpr()))
-        {
+        if (ignoreLocation(decl)) {
             return true;
         }
-        auto l1 = unwindToQObject(compat::getBeginLoc(decl));
-        if (l1.isValid() && l1 == unwindToQObject(compat::getEndLoc(decl))) {
+        if (!decl->isInlineSpecified()) {
             return true;
         }
-        SourceLocation inlineLoc;
-        unsigned n;
+        handleImplicitInline(decl) || handleNonExternalLinkage(decl);
+        return true;
+    }
+
+private:
+    bool removeInline(FunctionDecl const * decl, SourceLocation * inlineLoc) {
+        assert(inlineLoc != nullptr);
+        assert(inlineLoc->isInvalid());
+        unsigned n = {}; // avoid -Werror=maybe-uninitialized
         auto end = Lexer::getLocForEndOfToken(
             compiler.getSourceManager().getExpansionLoc(compat::getEndLoc(decl)), 0,
             compiler.getSourceManager(), compiler.getLangOpts());
@@ -58,7 +55,7 @@ public:
             }
             if (s == "inline") {
                 if (!compiler.getSourceManager().isMacroArgExpansion(loc)) {
-                    inlineLoc = loc;
+                    *inlineLoc = loc;
                 }
                 break;
             } else if (s == "#") {
@@ -75,8 +72,8 @@ public:
                 break;
             }
         }
-        if (rewriter != nullptr && inlineLoc.isValid()) {
-            for (auto loc = inlineLoc.getLocWithOffset(
+        if (rewriter != nullptr && inlineLoc->isValid()) {
+            for (auto loc = inlineLoc->getLocWithOffset(
                      std::max<unsigned>(n, 1));;)
             {
                 assert(loc != end);
@@ -95,20 +92,14 @@ public:
                 n += n2;
                 loc = loc.getLocWithOffset(n2);
             }
-            if (removeText(inlineLoc, n, RewriteOptions(RemoveLineIfEmpty))) {
+            if (removeText(*inlineLoc, n, RewriteOptions(RemoveLineIfEmpty))) {
                 return true;
             }
         }
-        report(
-            DiagnosticsEngine::Warning,
-            "function definition redundantly declared 'inline'",
-            inlineLoc.isValid() ? inlineLoc : compat::getBeginLoc(decl))
-            << decl->getSourceRange();
-        return true;
+        return false;
     }
 
-private:
-    SourceLocation unwindToQObject(SourceLocation const & loc) {
+    SourceLocation unwindTo(SourceLocation const & loc, StringRef name) {
         if (!loc.isMacroID()) {
             return SourceLocation();
         }
@@ -116,8 +107,67 @@ private:
         return
             (Lexer::getImmediateMacroName(
                 loc, compiler.getSourceManager(), compiler.getLangOpts())
-             == "Q_OBJECT")
-            ? l : unwindToQObject(l);
+             == name)
+            ? l : unwindTo(l, name);
+    }
+
+    bool isInMacroExpansion(FunctionDecl const * decl, StringRef name) {
+        auto loc = unwindTo(compat::getBeginLoc(decl), name);
+        return loc.isValid() && loc == unwindTo(compat::getEndLoc(decl), name);
+    }
+
+    bool handleImplicitInline(FunctionDecl const * decl) {
+        if (!(decl->doesThisDeclarationHaveABody() || decl->isExplicitlyDefaulted())
+            || !(decl->getLexicalDeclContext()->isRecord() || decl->isConstexpr()))
+        {
+            return false;
+        }
+        if (isInMacroExpansion(decl, "Q_OBJECT")) {
+            return true;
+        }
+        SourceLocation inlineLoc;
+        if (!removeInline(decl, &inlineLoc)) {
+            report(
+                DiagnosticsEngine::Warning,
+                "function definition redundantly declared 'inline'",
+                inlineLoc.isValid() ? inlineLoc : compat::getBeginLoc(decl))
+                << decl->getSourceRange();
+        }
+        return true;
+    }
+
+    bool handleNonExternalLinkage(FunctionDecl const * decl) {
+        if (decl->getLinkageInternal() >=
+#if CLANG_VERSION >= 40000
+            ModuleLinkage
+#else
+            ExternalLinkage
+#endif
+        )
+        {
+            return false;
+        }
+        if (!compiler.getSourceManager().isInMainFile(decl->getLocation())) {
+            // There *may* be benefit to "static inline" in include files (esp. in C code, where an
+            // inline function with external linkage still requires an external definition), so
+            // just ignore those for now:
+            return true;
+        }
+        if (isInMacroExpansion(decl, "G_DEFINE_TYPE")
+            || isInMacroExpansion(decl, "G_DEFINE_TYPE_WITH_CODE")
+            || isInMacroExpansion(decl, "G_DEFINE_TYPE_WITH_PRIVATE"))
+        {
+            return true;
+        }
+        SourceLocation inlineLoc;
+        if (!removeInline(decl, &inlineLoc)) {
+            report(
+                DiagnosticsEngine::Warning,
+                "function has no external linkage but is explicitly declared 'inline'",
+                inlineLoc.isValid() ? inlineLoc : compat::getBeginLoc(decl))
+                << decl->getSourceRange();
+        }
+        return true;
     }
 };
 
diff --git a/compilerplugins/clang/test/redundantinline.cxx b/compilerplugins/clang/test/redundantinline.cxx
index 038a1497d98e..f69e0a3b80cf 100644
--- a/compilerplugins/clang/test/redundantinline.cxx
+++ b/compilerplugins/clang/test/redundantinline.cxx
@@ -11,4 +11,14 @@
 
 S1::~S1() = default;
 
+static inline int f8() { return 0; } // expected-error {{function has no external linkage but is explicitly declared 'inline' [loplugin:redundantinline]}}
+
+namespace {
+
+static inline int f9() { return 0; } // expected-error {{function has no external linkage but is explicitly declared 'inline' [loplugin:redundantinline]}}
+
+}
+
+int main() { return f8() + f9(); }
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/test/redundantinline.hxx b/compilerplugins/clang/test/redundantinline.hxx
index e4efc5663e42..8e87ae05fcf7 100644
--- a/compilerplugins/clang/test/redundantinline.hxx
+++ b/compilerplugins/clang/test/redundantinline.hxx
@@ -52,27 +52,29 @@ void f3() {}
 S3::operator int() { return 0; }
 
 struct S4 {
-    inline S4() {} // expected-error {{[loplugin:redundantinline]}}
-    inline ~S4() { f1(); } // expected-error {{[loplugin:redundantinline]}}
+    inline S4() {} // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
+    inline ~S4() { f1(); } // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
-    inline void f1() { (void)this; } // expected-error {{[loplugin:redundantinline]}}
+    inline void f1() { (void)this; } // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
-    static inline void f2() {} // expected-error {{[loplugin:redundantinline]}}
+    static inline void f2() {} // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
-    inline void operator +() {} // expected-error {{[loplugin:redundantinline]}}
+    inline void operator +() {} // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
-    inline operator int() { return 0; } // expected-error {{[loplugin:redundantinline]}}
+    inline operator int() { return 0; } // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
-    friend inline void f4() {} // expected-error {{[loplugin:redundantinline]}}
+    friend inline void f4() {} // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 
     static constexpr int f5() { return 0; }
 
-    static constexpr inline int f6() { return 0; } // expected-error {{[loplugin:redundantinline]}}
+    static constexpr inline int f6() { return 0; } // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
 };
 
 constexpr int f5() { return 0; }
 
-constexpr inline int f6() { return 0; } // expected-error {{[loplugin:redundantinline]}}
+constexpr inline int f6() { return 0; } // expected-error {{function definition redundantly declared 'inline' [loplugin:redundantinline]}}
+
+static inline int f7() { return 0; }
 
 #endif