compilerplugins/clang/plugin.cxx | 177 ++++++++++++++
compilerplugins/clang/plugin.hxx | 4
compilerplugins/clang/redundantfcast.cxx | 30 --
connectivity/source/drivers/firebird/DatabaseMetaData.cxx | 1
drawinglayer/source/tools/emfppen.cxx | 4
sc/source/core/data/table2.cxx | 2
6 files changed, 200 insertions(+), 18 deletions(-)
New commits:
commit e63e769bd30800b72f4a1cdfc7222c0b64f3c770
Author: Stephan Bergmann <sberg...@redhat.com>
AuthorDate: Mon Apr 25 10:10:55 2022 +0200
Commit: Stephan Bergmann <sberg...@redhat.com>
CommitDate: Mon Apr 25 18:21:24 2022 +0200
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 <sberg...@redhat.com>
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;
}
diff --git a/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
b/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
index ab14d957f936..cfee4f41b81d 100644
--- a/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
+++ b/connectivity/source/drivers/firebird/DatabaseMetaData.cxx
@@ -1026,6 +1026,7 @@ uno::Reference< XResultSet > SAL_CALL
ODatabaseMetaData::getTypeInfo()
tmp.push_back(aRow);
return tmp;
}();
+ // [-loplugin:redundantfcast] false positive:
pResultSet->setRows(ODatabaseMetaDataResultSet::ORows(aResults));
return pResultSet;
}
diff --git a/drawinglayer/source/tools/emfppen.cxx
b/drawinglayer/source/tools/emfppen.cxx
index 7d999d61cc8b..b927ac186174 100644
--- a/drawinglayer/source/tools/emfppen.cxx
+++ b/drawinglayer/source/tools/emfppen.cxx
@@ -219,12 +219,16 @@ namespace emfplushelper
switch (dashStyle)
{
case EmfPlusLineStyleDash:
+ // [-loplugin:redundantfcast] false positive:
return drawinglayer::attribute::StrokeAttribute({ 3 * pw,
pw });
case EmfPlusLineStyleDot:
+ // [-loplugin:redundantfcast] false positive:
return drawinglayer::attribute::StrokeAttribute({ pw, pw
});
case EmfPlusLineStyleDashDot:
+ // [-loplugin:redundantfcast] false positive:
return drawinglayer::attribute::StrokeAttribute({ 3 * pw,
pw, pw, pw });
case EmfPlusLineStyleDashDotDot:
+ // [-loplugin:redundantfcast] false positive:
return drawinglayer::attribute::StrokeAttribute({ 3 * pw,
pw, pw, pw, pw, pw });
}
}
diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx
index e9b5bb371a85..55c11a3ce065 100644
--- a/sc/source/core/data/table2.cxx
+++ b/sc/source/core/data/table2.cxx
@@ -2891,6 +2891,7 @@ void ScTable::SetAttrEntries( SCCOL nStartCol, SCCOL
nEndCol, std::vector<ScAttr
// If we would like set all columns to same attrs, then change
only attrs for not existing columns
nEndCol = aCol.size() - 1;
for (SCCOL i = nStartCol; i <= nEndCol; i++)
+ // [-loplugin:redundantfcast] false positive:
aCol[i].SetAttrEntries( std::vector<ScAttrEntry>(vNewData));
aDefaultColData.SetAttrEntries(std::move(vNewData));
}
@@ -2904,6 +2905,7 @@ void ScTable::SetAttrEntries( SCCOL nStartCol, SCCOL
nEndCol, std::vector<ScAttr
{
CreateColumnIfNotExists( nEndCol );
for (SCCOL i = nStartCol; i < nEndCol; i++) // all but last need a copy
+ // [-loplugin:redundantfcast] false positive:
aCol[i].SetAttrEntries( std::vector<ScAttrEntry>(vNewData));
aCol[nEndCol].SetAttrEntries( std::move(vNewData));
}
