Introduce a better mechanism to suppress false loplugin warnings

...by annotating occurrences of false warnings with [-loplugin:<name>] comments
in source files and letting individual plugins opt-in to watch out for such
suppression annotations, rather than maintaining lists of excluded source files
in the individual plugins.  (See the new loplugin::Plugin::suppressWarningsAt.)

Instead of making all calls to loplugin::Plugin::report check for suppression
annotations, the intent is that this check will only be added opt-in to those
places in the plugins that are prone to emitting false warnings.  In general it
is better to have plugins that don't produce false warnings in the first place,
or at least let those warnings be addressed with trivial and harmless source
code modifications, avoiding the need for any suppression mechanism.

As a proof of concept, I have removed the exclude list from
loplugin:redundantfcast and instead annotated the relevant source code.  (And
thereby found that three of the six originally excluded files didn't need to be
excluded any more at all?)

For now, this mechanism looks for comments (both //... and /*...*/, even
documentation-style /**...*/) that overlap the current and/or the preceding
line, because at least for code controlled by clang-format it is often easier to
move comments to a line of their own, preceding the commented code.  Looking
also at the current line (and not only at the preceding one) opens the door for
erroneous over-eager annotation, where an annotation that was meant to address a
false warning on the current line would also silence a potentially true warning
on the following line.  This probably doesn't cause much trouble in practice,
but is up for potential change.

Change-Id: I91ce7a0e5248886a60b471b1a153867f16bb5cea
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/133365
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

3 files changed, 193 insertions, 18 deletions

diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index 8bf5b4333bdc..727eda589548 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -128,6 +128,183 @@ DiagnosticBuilder Plugin::report( DiagnosticsEngine::Level level, StringRef mess
     return handler.report( level, name, message, compiler, loc );
 }
 
+bool Plugin::suppressWarningAt(SourceLocation location) const {
+    auto const start = compiler.getSourceManager().getSpellingLoc(location);
+    auto const startInfo = compiler.getSourceManager().getDecomposedLoc(start);
+    auto invalid = false;
+    auto const buf = compiler.getSourceManager().getBufferData(startInfo.first, &invalid);
+    if (invalid) {
+        if (isDebugMode()) {
+            report(DiagnosticsEngine::Fatal, "failed to getBufferData", start);
+        }
+        return false;
+    }
+    auto const label = std::string("[-loplugin:").append(name).append("]");
+    // Look back to the beginning of the previous line:
+    auto loc = start;
+    auto locInfo = startInfo;
+    auto cur = loc;
+    enum class State { Normal, Slash, Comment };
+    auto state = State::Normal;
+    auto newlines = 0;
+    for (auto prev = cur;;) {
+        auto prevInfo = compiler.getSourceManager().getDecomposedLoc(prev);
+        if (prev == compiler.getSourceManager().getLocForStartOfFile(prevInfo.first)) {
+            if (state == State::Comment && isDebugMode()) {
+                report(
+                    DiagnosticsEngine::Fatal,
+                    "beginning of file while looking for beginning of comment", prev);
+            }
+            break;
+        }
+        Token tok;
+        if (Lexer::getRawToken(
+                Lexer::GetBeginningOfToken(
+                    prev.getLocWithOffset(-1), compiler.getSourceManager(), compiler.getLangOpts()),
+                tok, compiler.getSourceManager(), compiler.getLangOpts(), true))
+        {
+            if (isDebugMode()) {
+                report(
+                    DiagnosticsEngine::Fatal, "failed to getRawToken",
+                    prev.getLocWithOffset(-1));
+            }
+            break;
+        }
+        if (tok.getLocation() == cur) {
+            // Keep walking back, character by character, through whitespace preceding the current
+            // token, which Clang treats as nominally belonging to that token (so the above
+            // Lexer::getRawToken/Lexer::GetBeginningOfToken will have produced just the same tok
+            // again):
+            prev = prev.getLocWithOffset(-1);
+            continue;
+        }
+        cur = tok.getLocation();
+        prev = cur;
+        if (state == State::Comment) {
+            // Lexer::GetBeginningOfToken (at least towards Clang 15, still) only re-scans from the
+            // start of the current line, so if we saw the end of a multi-line /*...*/ comment, we
+            // saw that as individual '/' and '*' faux-tokens, at which point we must (hopefully?)
+            // actually be at the end of such a multi-line comment, so we keep walking back to the
+            // first '/*' we encounter (TODO: which still need not be the real start of the comment,
+            // if the comment contains embedded '/*', but we could determine that only if we
+            // re-scanned from the start of the file):
+            if (!tok.is(tok::comment)) {
+                continue;
+            }
+            SmallVector<char, 256> tmp;
+            bool invalid = false;
+            auto const spell = Lexer::getSpelling(
+                prev, tmp, compiler.getSourceManager(), compiler.getLangOpts(), &invalid);
+            if (invalid) {
+                if (isDebugMode()) {
+                    report(DiagnosticsEngine::Fatal, "failed to getSpelling", prev);
+                }
+            } else if (!spell.startswith("/*")) {
+                continue;
+            }
+        }
+        prevInfo = compiler.getSourceManager().getDecomposedLoc(prev);
+        auto const end = prev.getLocWithOffset(tok.getLength());
+        auto const endInfo = compiler.getSourceManager().getDecomposedLoc(end);
+        assert(prevInfo.first == endInfo .first);
+        assert(prevInfo.second <= endInfo.second);
+        assert(endInfo.first == locInfo.first);
+        // Whitespace between tokens is found at the end of prev, from end to loc (unless this is a
+        // multi-line comment, in which case the whitespace has already been inspected as the
+        // whitespace following the comment's final '/' faux-token):
+        StringRef ws;
+        if (state != State::Comment) {
+            assert(endInfo.second <= locInfo.second);
+            ws = buf.substr(endInfo.second, locInfo.second - endInfo.second);
+        }
+        for (std::size_t i = 0;;) {
+            auto const j = ws.find('\n', i);
+            if (j == StringRef::npos) {
+                break;
+            }
+            ++newlines;
+            if (newlines == 2) {
+                break;
+            }
+            i = j + 1;
+        }
+        if (newlines == 2) {
+            break;
+        }
+        auto str = buf.substr(prevInfo.second, endInfo.second - prevInfo.second);
+        if (tok.is(tok::comment) && str.contains(label)) {
+            return true;
+        }
+        for (std::size_t i = 0;;) {
+            auto const j = str.find('\n', i);
+            if (j == StringRef::npos) {
+                break;
+            }
+            ++newlines;
+            if (newlines == 2) {
+                break;
+            }
+            i = j + 1;
+        }
+        if (newlines == 2) {
+            break;
+        }
+        loc = prev;
+        locInfo = prevInfo;
+        switch (state) {
+        case State::Normal:
+            if (tok.is(tok::slash)) {
+                state = State::Slash;
+            }
+            break;
+        case State::Slash:
+            state = tok.is(tok::star) && ws.empty() ? State::Comment : State::Normal;
+                //TODO: check for "ws is only folding whitespace" rather than for `ws.empty()` (but
+                // then, we must not count newlines in that whitespace twice, first as part of the
+                // whitespace following the comment's semi-final '*' faux-token and then as part of
+                // the comment token's content)
+            break;
+        case State::Comment:
+            state = State::Normal;
+        }
+    }
+    // Look forward to the end of the current line:
+    loc = start;
+    locInfo = startInfo;
+    for (;;) {
+        Token tok;
+        if (Lexer::getRawToken(loc, tok, compiler.getSourceManager(), compiler.getLangOpts(), true))
+        {
+            if (isDebugMode()) {
+                report(DiagnosticsEngine::Fatal, "failed to getRawToken", loc);
+            }
+            break;
+        }
+        // Whitespace between tokens is found at the beginning, from loc to beg:
+        auto const beg = tok.getLocation();
+        auto const begInfo = compiler.getSourceManager().getDecomposedLoc(beg);
+        assert(begInfo.first == locInfo.first);
+        assert(begInfo.second >= locInfo.second);
+        if (buf.substr(locInfo.second, begInfo.second - locInfo.second).contains('\n')) {
+            break;
+        }
+        auto const next = beg.getLocWithOffset(tok.getLength());
+        auto const nextInfo = compiler.getSourceManager().getDecomposedLoc(next);
+        assert(nextInfo.first == begInfo.first);
+        assert(nextInfo.second >= begInfo.second);
+        auto const str = buf.substr(begInfo.second, nextInfo.second - begInfo.second);
+        if (tok.is(tok::comment) && str.contains(label)) {
+            return true;
+        }
+        if (tok.is(tok::eof) || str.contains('\n')) {
+            break;
+        }
+        loc = next;
+        locInfo = nextInfo;
+    }
+    return false;
+}
+
 void normalizeDotDotInFilePath( std::string & s )
 {
     for (std::string::size_type i = 0;;)
diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx
index 86ca8f825e02..cd272dc520f8 100644
--- a/compilerplugins/clang/plugin.hxx
+++ b/compilerplugins/clang/plugin.hxx
@@ -65,6 +65,10 @@ public:
     enum { isSharedPlugin = false };
 protected:
     DiagnosticBuilder report( DiagnosticsEngine::Level level, StringRef message, SourceLocation loc = SourceLocation()) const;
+    // Look at the line containing location and the previous line for any comments that overlap
+    // either of those two lines and that contain "[-loplugin:<name>]" (with the name of this
+    // plugin), indicating that warnings from this plugin should be suppressed here:
+    bool suppressWarningAt(SourceLocation location) const;
     bool ignoreLocation( SourceLocation loc ) const
     { return handler.ignoreLocation(loc); }
     bool ignoreLocation( const Decl* decl ) const;
diff --git a/compilerplugins/clang/redundantfcast.cxx b/compilerplugins/clang/redundantfcast.cxx
index aed71e8783f9..e78f2cf976ab 100644
--- a/compilerplugins/clang/redundantfcast.cxx
+++ b/compilerplugins/clang/redundantfcast.cxx
@@ -56,9 +56,15 @@ public:
             return true;
         }
         if (m_Seen.insert(cxxFunctionalCastExpr->getExprLoc()).second)
+        {
+            if (suppressWarningAt(cxxFunctionalCastExpr->getBeginLoc()))
+            {
+                return true;
+            }
             report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
                    cxxFunctionalCastExpr->getExprLoc())
                 << t2 << t1 << cxxFunctionalCastExpr->getSourceRange();
+        }
         return true;
     }
 
@@ -310,9 +316,15 @@ public:
             return true;
 
         if (m_Seen.insert(expr->getExprLoc()).second)
+        {
+            if (suppressWarningAt(expr->getBeginLoc()))
+            {
+                return true;
+            }
             report(DiagnosticsEngine::Warning, "redundant functional cast from %0 to %1",
                    expr->getExprLoc())
                 << t2 << t1 << expr->getSourceRange();
+        }
         return true;
     }
 
@@ -320,24 +332,6 @@ public:
     {
         if (!compiler.getLangOpts().CPlusPlus)
             return false;
-        std::string fn = handler.getMainFileName().str();
-        loplugin::normalizeDotDotInFilePath(fn);
-        // necessary on some other platforms
-        if (fn == SRCDIR "/sal/osl/unx/socket.cxx")
-            return false;
-        // compile-time check of constant
-        if (fn == SRCDIR "/bridges/source/jni_uno/jni_bridge.cxx")
-            return false;
-        // TODO constructing a temporary to pass to a && param
-        if (fn == SRCDIR "/sc/source/ui/view/viewfunc.cxx"
-            || fn == SRCDIR "/sc/source/core/data/table2.cxx")
-            return false;
-        // tdf#145203: FIREBIRD cannot create a table
-        if (fn == SRCDIR "/connectivity/source/drivers/firebird/DatabaseMetaData.cxx")
-            return false;
-        // false positive during using constructor drawinglayer::attribute::StrokeAttribute({ 3 * pw, pw })
-        if (fn == SRCDIR "/drawinglayer/source/tools/emfppen.cxx")
-            return false;
         return true;
     }