Extend loplugin:salbool to loplugin:fakebool

...checking for unnecessary uses of more "fake bool" types.

In the past, some of the checks involving the types of variables or data
members, or the return types of functions, issued warnings that required
surrounding code to be changed too (e.g., when changing the signature of a
function whose address was taken).  These checks have been tightened now to not
warn in such cases (which avoids warnings that require changes to additional
code, or changes that might even be impossible to make, at the cost of being
less aggressive about removing all unnecessary uses of those "fake bool" types).

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

2 files changed, 477 insertions, 249 deletions

diff --git a/compilerplugins/clang/salbool.cxx b/compilerplugins/clang/fakebool.cxx
index fdadfc6b795a..1dbb535ceef9 100644
--- a/compilerplugins/clang/salbool.cxx
+++ b/compilerplugins/clang/fakebool.cxx
@@ -10,27 +10,167 @@
 #include <algorithm>
 #include <cassert>
 #include <limits>
-#include <set>
+#include <map>
 #include <string>
 
 #include "clang/AST/Attr.h"
 
 #include "check.hxx"
 #include "compat.hxx"
+#include "functionaddress.hxx"
 #include "plugin.hxx"
 
 namespace {
 
-bool isSalBool(QualType type) {
+// BEGIN code copied from LLVM's clang/lib/Sema/Sema.cpp
+
+typedef llvm::DenseMap<const CXXRecordDecl*, bool> RecordCompleteMap;
+
+/// Returns true, if all methods and nested classes of the given
+/// CXXRecordDecl are defined in this translation unit.
+///
+/// Should only be called from ActOnEndOfTranslationUnit so that all
+/// definitions are actually read.
+static bool MethodsAndNestedClassesComplete(const CXXRecordDecl *RD,
+                                            RecordCompleteMap &MNCComplete) {
+  RecordCompleteMap::iterator Cache = MNCComplete.find(RD);
+  if (Cache != MNCComplete.end())
+    return Cache->second;
+  if (!RD->isCompleteDefinition())
+    return false;
+  bool Complete = true;
+  for (DeclContext::decl_iterator I = RD->decls_begin(),
+                                  E = RD->decls_end();
+       I != E && Complete; ++I) {
+    if (const CXXMethodDecl *M = dyn_cast<CXXMethodDecl>(*I))
+      Complete = M->isDefined() || M->isDefaulted() ||
+                 (M->isPure() && !isa<CXXDestructorDecl>(M));
+    else if (const FunctionTemplateDecl *F = dyn_cast<FunctionTemplateDecl>(*I))
+      // If the template function is marked as late template parsed at this
+      // point, it has not been instantiated and therefore we have not
+      // performed semantic analysis on it yet, so we cannot know if the type
+      // can be considered complete.
+      Complete = !F->getTemplatedDecl()->isLateTemplateParsed() &&
+                  F->getTemplatedDecl()->isDefined();
+    else if (const CXXRecordDecl *R = dyn_cast<CXXRecordDecl>(*I)) {
+      if (R->isInjectedClassName())
+        continue;
+      if (R->hasDefinition())
+        Complete = MethodsAndNestedClassesComplete(R->getDefinition(),
+                                                   MNCComplete);
+      else
+        Complete = false;
+    }
+  }
+  MNCComplete[RD] = Complete;
+  return Complete;
+}
+
+/// Returns true, if the given CXXRecordDecl is fully defined in this
+/// translation unit, i.e. all methods are defined or pure virtual and all
+/// friends, friend functions and nested classes are fully defined in this
+/// translation unit.
+///
+/// Should only be called from ActOnEndOfTranslationUnit so that all
+/// definitions are actually read.
+static bool IsRecordFullyDefined(const CXXRecordDecl *RD,
+                                 RecordCompleteMap &RecordsComplete,
+                                 RecordCompleteMap &MNCComplete) {
+  RecordCompleteMap::iterator Cache = RecordsComplete.find(RD);
+  if (Cache != RecordsComplete.end())
+    return Cache->second;
+  bool Complete = MethodsAndNestedClassesComplete(RD, MNCComplete);
+  for (CXXRecordDecl::friend_iterator I = RD->friend_begin(),
+                                      E = RD->friend_end();
+       I != E && Complete; ++I) {
+    // Check if friend classes and methods are complete.
+    if (TypeSourceInfo *TSI = (*I)->getFriendType()) {
+      // Friend classes are available as the TypeSourceInfo of the FriendDecl.
+      if (CXXRecordDecl *FriendD = TSI->getType()->getAsCXXRecordDecl())
+        Complete = MethodsAndNestedClassesComplete(FriendD, MNCComplete);
+      else
+        Complete = false;
+    } else {
+      // Friend functions are available through the NamedDecl of FriendDecl.
+      if (const FunctionDecl *FD =
+          dyn_cast<FunctionDecl>((*I)->getFriendDecl()))
+        Complete = FD->isDefined();
+      else
+        // This is a template friend, give up.
+        Complete = false;
+    }
+  }
+  RecordsComplete[RD] = Complete;
+  return Complete;
+}
+
+RecordCompleteMap RecordsComplete;
+RecordCompleteMap MNCComplete;
+
+// END code copied from LLVM's clang/lib/Sema/Sema.cpp
+
+// Is all code that could see `decl` defined in this TU?
+bool isAllRelevantCodeDefined(NamedDecl const * decl) {
+    switch (decl->getAccess()) {
+    case AS_protected:
+        if (!cast<CXXRecordDecl>(decl->getDeclContext())->hasAttr<FinalAttr>()) {
+            break;
+        }
+        LLVM_FALLTHROUGH;
+    case AS_private:
+        if (IsRecordFullyDefined(
+                cast<CXXRecordDecl>(decl->getDeclContext()), RecordsComplete, MNCComplete))
+        {
+            return true;
+        }
+        break;
+    default:
+        break;
+    }
+    return !decl->isExternallyVisible();
+}
+
+enum FakeBoolKind {
+    FBK_No,
+    FBK_BOOL, FBK_First = FBK_BOOL,
+    FBK_Boolean, FBK_FT_Bool, FBK_FcBool, FBK_GLboolean, FBK_NPBool, FBK_TW_BOOL, FBK_UBool,
+    FBK_boolean, FBK_dbus_bool_t, FBK_gboolean, FBK_hb_boot_t, FBK_jboolean, FBK_my_bool,
+    FBK_sal_Bool,
+    FBK_End };
+    // matches loplugin::TypeCheck::AnyBoolean (compilerplugins/clang/check.hxx)
+
+StringRef getName(FakeBoolKind k) {
+    static constexpr llvm::StringLiteral names[] = {
+        "BOOL", "Boolean", "FT_Bool", "FcBool", "GLboolean", "NPBool", "TW_BOOL", "UBool",
+        "boolean", "dbus_bool_t", "gboolean", "hb_boot_t", "jboolean", "my_bool", "sal_Bool"};
+    assert(k >= FBK_First && k < FBK_End);
+    return names[k - FBK_First];
+}
+
+FakeBoolKind isFakeBool(QualType type) {
     TypedefType const * t = type->getAs<TypedefType>();
-    return t != nullptr && t->getDecl()->getName() == "sal_Bool";
+    if (t != nullptr) {
+        auto const name = t->getDecl()->getName();
+        for (int i = FBK_First; i != FBK_End; ++i) {
+            auto const k = FakeBoolKind(i);
+            if (name == getName(k)) {
+                return k;
+            }
+        }
+    }
+    return FBK_No;
 }
 
-bool isSalBoolArray(QualType type) {
+FakeBoolKind isFakeBoolArray(QualType type) {
     auto t = type->getAsArrayTypeUnsafe();
-    return t != nullptr
-        && (isSalBool(t->getElementType())
-            || isSalBoolArray(t->getElementType()));
+    if (t == nullptr) {
+        return FBK_No;
+    }
+    auto const k = isFakeBool(t->getElementType());
+    if (k != FBK_No) {
+        return k;
+    }
+    return isFakeBoolArray(t->getElementType());
 }
 
 // It appears that, given a function declaration, there is no way to determine
@@ -78,27 +218,27 @@ BoolOverloadKind isBoolOverloadOf(
     if (!mustBeDeleted || f->isDeleted()) {
         unsigned n = decl->getNumParams();
         if (f->getNumParams() == n) {
-            bool hasSB = false;
+            bool hasFB = false;
             for (unsigned i = 0; i != n; ++i) {
                 QualType t1 { decl->getParamDecl(i)->getType() };
-                bool isSB = isSalBool(t1);
-                bool isSBRef = !isSB && t1->isReferenceType()
-                    && isSalBool(t1.getNonReferenceType());
+                bool isFB = isFakeBool(t1) != FBK_No;
+                bool isFBRef = !isFB && t1->isReferenceType()
+                    && isFakeBool(t1.getNonReferenceType()) != FBK_No;
                 QualType t2 { f->getParamDecl(i)->getType() };
-                if (!(isSB
+                if (!(isFB
                       ? t2->isBooleanType()
-                      : isSBRef
+                      : isFBRef
                       ? (t2->isReferenceType()
                          && t2.getNonReferenceType()->isBooleanType())
                       : t2.getCanonicalType() == t1.getCanonicalType()))
                 {
                     return BoolOverloadKind::CheckNext;
                 }
-                hasSB |= isSB || isSBRef;
+                hasFB |= isFB || isFBRef;
             }
-            return hasSB ? BoolOverloadKind::Yes : BoolOverloadKind::No;
+            return hasFB ? BoolOverloadKind::Yes : BoolOverloadKind::No;
                 // cheaply protect against the case where decl would have no
-                // sal_Bool parameters at all and would match itself
+                // fake bool parameters at all and would match itself
         }
     }
     return BoolOverloadKind::CheckNext;
@@ -142,12 +282,12 @@ bool hasBoolOverload(FunctionDecl const * decl, bool mustBeDeleted) {
     return false;
 }
 
-class SalBool:
-    public loplugin::FilteringRewritePlugin<SalBool>
+class FakeBool:
+    public loplugin::FunctionAddress<loplugin::FilteringRewritePlugin<FakeBool>>
 {
 public:
-    explicit SalBool(loplugin::InstantiationData const & data):
-        FilteringRewritePlugin(data) {}
+    explicit FakeBool(loplugin::InstantiationData const & data):
+        FunctionAddress(data) {}
 
     virtual void run() override;
 
@@ -190,16 +330,20 @@ private:
 
     bool isInSpecialMainFile(SourceLocation spellingLocation) const;
 
-    bool rewrite(SourceLocation location);
+    bool rewrite(SourceLocation location, FakeBoolKind kind);
 
-    std::set<VarDecl const *> varDecls_;
+    std::map<VarDecl const *, FakeBoolKind> varDecls_;
+    std::map<FieldDecl const *, FakeBoolKind> fieldDecls_;
+    std::map<ParmVarDecl const *, FakeBoolKind> parmVarDecls_;
+    std::map<FunctionDecl const *, FakeBoolKind> functionDecls_;
     unsigned int externCContexts_ = 0;
 };
 
-void SalBool::run() {
+void FakeBool::run() {
     if (compiler.getLangOpts().CPlusPlus) {
         TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
-        for (auto decl: varDecls_) {
+        for (auto const dcl: varDecls_) {
+            auto const decl = dcl.first; auto const fbk = dcl.second;
             SourceLocation loc { compat::getBeginLoc(decl) };
             TypeSourceInfo * tsi = decl->getTypeSourceInfo();
             if (tsi != nullptr) {
@@ -221,7 +365,95 @@ void SalBool::run() {
                         std::string s {
                             compiler.getSourceManager().getCharacterData(l),
                             n };
-                        if (s == "sal_Bool") {
+                        if (s == getName(fbk)) {
+                            loc = l;
+                            break;
+                        }
+                        if (l == end) {
+                            break;
+                        }
+                        l = l.getLocWithOffset(std::max<unsigned>(n, 1));
+                    }
+                }
+            }
+            if (!rewrite(loc, fbk)) {
+                report(
+                    DiagnosticsEngine::Warning,
+                    "VarDecl, use \"bool\" instead of %0", loc)
+                    << decl->getType().getLocalUnqualifiedType()
+                    << decl->getSourceRange();
+            }
+        }
+        for (auto const dcl: fieldDecls_) {
+            auto const decl = dcl.first; auto const fbk = dcl.second;
+            SourceLocation loc { compat::getBeginLoc(decl) };
+            TypeSourceInfo * tsi = decl->getTypeSourceInfo();
+            if (tsi != nullptr) {
+                SourceLocation l {
+                    compiler.getSourceManager().getExpansionLoc(
+                        tsi->getTypeLoc().getBeginLoc()) };
+                SourceLocation end {
+                    compiler.getSourceManager().getExpansionLoc(
+                        tsi->getTypeLoc().getEndLoc()) };
+                assert(l.isFileID() && end.isFileID());
+                if (l == end
+                    || compiler.getSourceManager().isBeforeInTranslationUnit(
+                        l, end))
+                {
+                    for (;;) {
+                        unsigned n = Lexer::MeasureTokenLength(
+                            l, compiler.getSourceManager(),
+                            compiler.getLangOpts());
+                        std::string s {
+                            compiler.getSourceManager().getCharacterData(l),
+                            n };
+                        if (s == getName(fbk)) {
+                            loc = l;
+                            break;
+                        }
+                        if (l == end) {
+                            break;
+                        }
+                        l = l.getLocWithOffset(std::max<unsigned>(n, 1));
+                    }
+                }
+            }
+            if (!rewrite(loc, fbk)) {
+                report(
+                    DiagnosticsEngine::Warning,
+                    "FieldDecl, use \"bool\" instead of %0", loc)
+                    << decl->getType().getLocalUnqualifiedType() << decl->getSourceRange();
+            }
+        }
+        auto const ignoredFns = getFunctionsWithAddressTaken();
+        for (auto const dcl: parmVarDecls_) {
+            auto const decl = dcl.first; auto const fbk = dcl.second;
+            FunctionDecl const * f = cast<FunctionDecl>(decl->getDeclContext())->getCanonicalDecl();
+            if (ignoredFns.find(f) != ignoredFns.end()) {
+                continue;
+            }
+            SourceLocation loc { compat::getBeginLoc(decl) };
+            TypeSourceInfo * tsi = decl->getTypeSourceInfo();
+            if (tsi != nullptr) {
+                SourceLocation l {
+                    compiler.getSourceManager().getExpansionLoc(
+                        tsi->getTypeLoc().getBeginLoc()) };
+                SourceLocation end {
+                    compiler.getSourceManager().getExpansionLoc(
+                        tsi->getTypeLoc().getEndLoc()) };
+                assert(l.isFileID() && end.isFileID());
+                if (l == end
+                    || (compiler.getSourceManager()
+                        .isBeforeInTranslationUnit(l, end)))
+                {
+                    for (;;) {
+                        unsigned n = Lexer::MeasureTokenLength(
+                            l, compiler.getSourceManager(),
+                            compiler.getLangOpts());
+                        std::string s {
+                            compiler.getSourceManager().getCharacterData(l),
+                            n };
+                        if (s == getName(fbk)) {
                             loc = l;
                             break;
                         }
@@ -232,31 +464,114 @@ void SalBool::run() {
                     }
                 }
             }
-            if (!rewrite(loc)) {
+            // Only rewrite declarations in include files if a
+            // definition is also seen, to avoid compilation of a
+            // definition (in a main file only processed later) to fail
+            // with a "mismatch" error before the rewriter had a chance
+            // to act upon the definition (but use the heuristic of
+            // assuming pure virtual functions do not have definitions);
+            // also, do not automatically rewrite functions that could
+            // implicitly override depend base functions (and thus stop
+            // doing so after the rewrite; note that this is less
+            // dangerous for return types than for parameter types,
+            // where the function would still implicitly override and
+            // cause a compilation error due to the incompatible return
+            // type):
+            OverrideKind k = getOverrideKind(f);
+            if (!((compiler.getSourceManager().isInMainFile(
+                       compiler.getSourceManager().getSpellingLoc(
+                           dyn_cast<FunctionDecl>(
+                               decl->getDeclContext())
+                           ->getNameInfo().getLoc()))
+                   || f->isDefined() || f->isPure())
+                  && k != OverrideKind::MAYBE && rewrite(loc, fbk)))
+            {
                 report(
                     DiagnosticsEngine::Warning,
-                    "VarDecl, use \"bool\" instead of \"sal_Bool\"", loc)
+                    ("ParmVarDecl, use \"bool\" instead of"
+                     " %0%1"),
+                    loc)
+                    << decl->getType().getNonReferenceType().getLocalUnqualifiedType()
+                    << (k == OverrideKind::MAYBE
+                        ? (" (unless this member function overrides a"
+                           " dependent base member function, even"
+                           " though it is not marked 'override')")
+                        : "")
+                    << decl->getSourceRange();
+            }
+        }
+        for (auto const dcl: functionDecls_) {
+            auto const decl = dcl.first; auto const fbk = dcl.second;
+            FunctionDecl const * f = decl->getCanonicalDecl();
+            if (ignoredFns.find(f) != ignoredFns.end()) {
+                continue;
+            }
+            SourceLocation loc { compat::getBeginLoc(decl) };
+            SourceLocation l { compiler.getSourceManager().getExpansionLoc(
+                loc) };
+            SourceLocation end { compiler.getSourceManager().getExpansionLoc(
+                decl->getNameInfo().getLoc()) };
+            assert(l.isFileID() && end.isFileID());
+            if (compiler.getSourceManager().isBeforeInTranslationUnit(l, end)) {
+                while (l != end) {
+                    unsigned n = Lexer::MeasureTokenLength(
+                        l, compiler.getSourceManager(), compiler.getLangOpts());
+                    std::string s {
+                        compiler.getSourceManager().getCharacterData(l), n };
+                    if (s == getName(fbk)) {
+                        loc = l;
+                        break;
+                    }
+                    l = l.getLocWithOffset(std::max<unsigned>(n, 1));
+                }
+            }
+            // Only rewrite declarations in include files if a definition is
+            // also seen, to avoid compilation of a definition (in a main file
+            // only processed later) to fail with a "mismatch" error before the
+            // rewriter had a chance to act upon the definition (but use the
+            // heuristic of assuming pure virtual functions do not have
+            // definitions):
+            if (!((compiler.getSourceManager().isInMainFile(
+                       compiler.getSourceManager().getSpellingLoc(
+                           decl->getNameInfo().getLoc()))
+                   || f->isDefined() || f->isPure())
+                  && rewrite(loc, fbk)))
+            {
+                report(
+                    DiagnosticsEngine::Warning,
+                    "use \"bool\" instead of %0 as return type%1",
+                    loc)
+                    << decl->getReturnType().getNonReferenceType().getLocalUnqualifiedType()
+                    << (getOverrideKind(f) == OverrideKind::MAYBE
+                        ? (" (unless this member function overrides a dependent"
+                           " base member function, even though it is not marked"
+                           " 'override')")
+                        : "")
                     << decl->getSourceRange();
             }
         }
     }
 }
 
-bool SalBool::VisitUnaryAddrOf(UnaryOperator const * op) {
+bool FakeBool::VisitUnaryAddrOf(UnaryOperator const * op) {
+    FunctionAddress::VisitUnaryAddrOf(op);
     Expr const * e1 = op->getSubExpr()->IgnoreParenCasts();
-    if (isSalBool(e1->getType())) {
-        DeclRefExpr const * e2 = dyn_cast<DeclRefExpr>(e1);
-        if (e2 != nullptr) {
+    if (isFakeBool(e1->getType()) != FBK_No) {
+        if (DeclRefExpr const * e2 = dyn_cast<DeclRefExpr>(e1)) {
             VarDecl const * d = dyn_cast<VarDecl>(e2->getDecl());
             if (d != nullptr) {
                 varDecls_.erase(d);
             }
+        } else if (auto const e3 = dyn_cast<MemberExpr>(e1)) {
+            if (auto const d = dyn_cast<FieldDecl>(e3->getMemberDecl())) {
+                fieldDecls_.erase(d);
+            }
         }
     }
     return true;
 }
 
-bool SalBool::VisitCallExpr(CallExpr * expr) {
+bool FakeBool::VisitCallExpr(CallExpr * expr) {
     Decl const * d = expr->getCalleeDecl();
     FunctionProtoType const * ft = nullptr;
     if (d != nullptr) {
@@ -290,7 +605,7 @@ bool SalBool::VisitCallExpr(CallExpr * expr) {
             bool b = false;
             if (t->isLValueReferenceType()) {
                 t = t.getNonReferenceType();
-                b = !t.isConstQualified() && isSalBool(t);
+                b = !t.isConstQualified() && isFakeBool(t) != FBK_No;
             } else if (t->isPointerType()) {
                 for (;;) {
                     auto t2 = t->getAs<clang::PointerType>();
@@ -299,16 +614,19 @@ bool SalBool::VisitCallExpr(CallExpr * expr) {
                     }
                     t = t2->getPointeeType();
                 }
-                b = isSalBool(t);
+                b = isFakeBool(t) != FBK_No;
             }
             if (b && i < expr->getNumArgs()) {
-                DeclRefExpr * ref = dyn_cast<DeclRefExpr>(
-                    expr->getArg(i)->IgnoreParenImpCasts());
-                if (ref != nullptr) {
+                auto const e1 = expr->getArg(i)->IgnoreParenImpCasts();
+                if (DeclRefExpr * ref = dyn_cast<DeclRefExpr>(e1)) {
                     VarDecl const * d = dyn_cast<VarDecl>(ref->getDecl());
                     if (d != nullptr) {
                         varDecls_.erase(d);
                     }
+                } else if (auto const e2 = dyn_cast<MemberExpr>(e1)) {
+                    if (auto const d = dyn_cast<FieldDecl>(e2->getMemberDecl())) {
+                        fieldDecls_.erase(d);
+                    }
                 }
             }
         }
@@ -316,11 +634,12 @@ bool SalBool::VisitCallExpr(CallExpr * expr) {
     return true;
 }
 
-bool SalBool::VisitCStyleCastExpr(CStyleCastExpr * expr) {
+bool FakeBool::VisitCStyleCastExpr(CStyleCastExpr * expr) {
     if (ignoreLocation(expr)) {
         return true;
     }
-    if (isSalBool(expr->getType())) {
+    auto const k = isFakeBool(expr->getType());
+    if (k != FBK_No) {
         SourceLocation loc { compat::getBeginLoc(expr) };
         while (compiler.getSourceManager().isMacroArgExpansion(loc)) {
             loc = compiler.getSourceManager().getImmediateMacroCallerLoc(loc);
@@ -328,7 +647,7 @@ bool SalBool::VisitCStyleCastExpr(CStyleCastExpr * expr) {
         if (compiler.getSourceManager().isMacroBodyExpansion(loc)) {
             StringRef name { Lexer::getImmediateMacroName(
                 loc, compiler.getSourceManager(), compiler.getLangOpts()) };
-            if (name == "sal_False" || name == "sal_True") {
+            if (k == FBK_sal_Bool && (name == "sal_False" || name == "sal_True")) {
                 auto callLoc = compiler.getSourceManager()
                     .getImmediateMacroCallerLoc(loc);
                 if (!isSharedCAndCppCode(callLoc)) {
@@ -345,7 +664,7 @@ bool SalBool::VisitCStyleCastExpr(CStyleCastExpr * expr) {
                         // arguments to CPPUNIT_ASSERT_EQUAL:
                         return true;
                     }
-                    bool b = name == "sal_True";
+                    bool b = k == FBK_sal_Bool && name == "sal_True";
                     if (rewriter != nullptr) {
                         auto callSpellLoc = compiler.getSourceManager()
                             .getSpellingLoc(callLoc);
@@ -380,29 +699,34 @@ bool SalBool::VisitCStyleCastExpr(CStyleCastExpr * expr) {
     return true;
 }
 
-bool SalBool::VisitCXXStaticCastExpr(CXXStaticCastExpr * expr) {
+bool FakeBool::VisitCXXStaticCastExpr(CXXStaticCastExpr * expr) {
     if (ignoreLocation(expr)) {
         return true;
     }
-    if (isSalBool(expr->getType())
-        && !isInSpecialMainFile(
+    auto const k = isFakeBool(expr->getType());
+    if (k == FBK_No) {
+        return true;
+    }
+    if (k == FBK_sal_Bool
+        && isInSpecialMainFile(
             compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(expr))))
     {
-        report(
-            DiagnosticsEngine::Warning,
-            "CXXStaticCastExpr, suspicious cast from %0 to %1",
-            compat::getBeginLoc(expr))
-            << expr->getSubExpr()->IgnoreParenImpCasts()->getType()
-            << expr->getType() << expr->getSourceRange();
+        return true;
     }
+    report(
+        DiagnosticsEngine::Warning,
+        "CXXStaticCastExpr, suspicious cast from %0 to %1",
+        compat::getBeginLoc(expr))
+        << expr->getSubExpr()->IgnoreParenImpCasts()->getType()
+        << expr->getType() << expr->getSourceRange();
     return true;
 }
 
-bool SalBool::VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr * expr) {
+bool FakeBool::VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr * expr) {
     if (ignoreLocation(expr)) {
         return true;
     }
-    if (isSalBool(expr->getType())) {
+    if (isFakeBool(expr->getType()) != FBK_No) {
         report(
             DiagnosticsEngine::Warning,
             "CXXFunctionalCastExpr, suspicious cast from %0 to %1",
@@ -413,11 +737,13 @@ bool SalBool::VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr * expr) {
     return true;
 }
 
-bool SalBool::VisitImplicitCastExpr(ImplicitCastExpr * expr) {
+bool FakeBool::VisitImplicitCastExpr(ImplicitCastExpr * expr) {
+    FunctionAddress::VisitImplicitCastExpr(expr);
     if (ignoreLocation(expr)) {
         return true;
     }
-    if (!isSalBool(expr->getType())) {
+    auto const k = isFakeBool(expr->getType());
+    if (k == FBK_No) {
         return true;
     }
     auto l = compat::getBeginLoc(expr);
@@ -427,7 +753,11 @@ bool SalBool::VisitImplicitCastExpr(ImplicitCastExpr * expr) {
     if (compiler.getSourceManager().isMacroBodyExpansion(l)) {
         auto n = Lexer::getImmediateMacroName(
             l, compiler.getSourceManager(), compiler.getLangOpts());
-        if (n == "sal_False" || n == "sal_True") {
+        if ((k == FBK_GLboolean && (n == "GL_FALSE" || n == "GL_TRUE"))
+            || (k == FBK_UBool && (n == "FALSE" || n == "TRUE"))
+            || (k == FBK_jboolean && (n == "JNI_FALSE" || n == "JNI_TRUE"))
+            || (k == FBK_sal_Bool && (n == "sal_False" || n == "sal_True")))
+        {
             return true;
         }
     }
@@ -454,13 +784,13 @@ bool SalBool::VisitImplicitCastExpr(ImplicitCastExpr * expr) {
         }
     }
     report(
-        DiagnosticsEngine::Warning, "conversion from %0 to sal_Bool",
+        DiagnosticsEngine::Warning, "conversion from %0 to %1",
         compat::getBeginLoc(expr))
-        << t << expr->getSourceRange();
+        << t << expr->getType() << expr->getSourceRange();
     return true;
 }
 
-bool SalBool::VisitReturnStmt(ReturnStmt const * stmt) {
+bool FakeBool::VisitReturnStmt(ReturnStmt const * stmt) {
     // Just enough to avoid warnings in rtl_getUriCharClass (sal/rtl/uri.cxx),
     // which has
     //
@@ -488,7 +818,7 @@ bool SalBool::VisitReturnStmt(ReturnStmt const * stmt) {
         }
         t = t2->getPointeeType();
     }
-    if (!isSalBool(t)) {
+    if (isFakeBool(t) != FBK_sal_Bool) {
         return true;
     }
     auto e2 = dyn_cast<ArraySubscriptExpr>(e->IgnoreParenImpCasts());
@@ -507,92 +837,30 @@ bool SalBool::VisitReturnStmt(ReturnStmt const * stmt) {
     return true;
 }
 
-bool SalBool::WalkUpFromParmVarDecl(ParmVarDecl const * decl) {
+bool FakeBool::WalkUpFromParmVarDecl(ParmVarDecl const * decl) {
     return VisitParmVarDecl(decl);
 }
 
-bool SalBool::VisitParmVarDecl(ParmVarDecl const * decl) {
+bool FakeBool::VisitParmVarDecl(ParmVarDecl const * decl) {
     if (ignoreLocation(decl)) {
         return true;
     }
-    if (isSalBool(decl->getType().getNonReferenceType())) {
+    auto const fbk = isFakeBool(decl->getType().getNonReferenceType());
+    if (fbk != FBK_No) {
         FunctionDecl const * f = dyn_cast<FunctionDecl>(decl->getDeclContext());
         if (f != nullptr) { // e.g.: typedef sal_Bool (* FuncPtr )( sal_Bool );
             f = f->getCanonicalDecl();
-            if (!(hasCLanguageLinkageType(f)
-                  || (isInUnoIncludeFile(f)
-                      && (!f->isInlined() || f->hasAttr<DeprecatedAttr>()
-                          || decl->getType()->isReferenceType()
-                          || hasBoolOverload(f, false)))
-                  || f->isDeleted() || hasBoolOverload(f, true)))
+            if (isAllRelevantCodeDefined(f)
+                && !(hasCLanguageLinkageType(f)
+                     || (fbk == FBK_sal_Bool && isInUnoIncludeFile(f)
+                         && (!f->isInlined() || f->hasAttr<DeprecatedAttr>()
+                             || decl->getType()->isReferenceType()
+                             || hasBoolOverload(f, false)))
+                     || f->isDeleted() || hasBoolOverload(f, true)))
             {
                 OverrideKind k = getOverrideKind(f);
                 if (k != OverrideKind::YES) {
-                    SourceLocation loc { compat::getBeginLoc(decl) };
-                    TypeSourceInfo * tsi = decl->getTypeSourceInfo();
-                    if (tsi != nullptr) {
-                        SourceLocation l {
-                            compiler.getSourceManager().getExpansionLoc(
-                                tsi->getTypeLoc().getBeginLoc()) };
-                        SourceLocation end {
-                            compiler.getSourceManager().getExpansionLoc(
-                                tsi->getTypeLoc().getEndLoc()) };
-                        assert(l.isFileID() && end.isFileID());
-                        if (l == end
-                            || (compiler.getSourceManager()
-                                .isBeforeInTranslationUnit(l, end)))
-                        {
-                            for (;;) {
-                                unsigned n = Lexer::MeasureTokenLength(
-                                    l, compiler.getSourceManager(),
-                                    compiler.getLangOpts());
-                                std::string s {
-                                    compiler.getSourceManager().getCharacterData(l),
-                                        n };
-                                if (s == "sal_Bool") {
-                                    loc = l;
-                                    break;
-                                }
-                                if (l == end) {
-                                    break;
-                                }
-                                l = l.getLocWithOffset(std::max<unsigned>(n, 1));
-                            }
-                        }
-                    }
-                    // Only rewrite declarations in include files if a
-                    // definition is also seen, to avoid compilation of a
-                    // definition (in a main file only processed later) to fail
-                    // with a "mismatch" error before the rewriter had a chance
-                    // to act upon the definition (but use the heuristic of
-                    // assuming pure virtual functions do not have definitions);
-                    // also, do not automatically rewrite functions that could
-                    // implicitly override depend base functions (and thus stop
-                    // doing so after the rewrite; note that this is less
-                    // dangerous for return types than for parameter types,
-                    // where the function would still implicitly override and
-                    // cause a compilation error due to the incompatible return
-                    // type):
-                    if (!((compiler.getSourceManager().isInMainFile(
-                               compiler.getSourceManager().getSpellingLoc(
-                                   dyn_cast<FunctionDecl>(
-                                       decl->getDeclContext())
-                                   ->getNameInfo().getLoc()))
-                           || f->isDefined() || f->isPure())
-                          && k != OverrideKind::MAYBE && rewrite(loc)))
-                    {
-                        report(
-                            DiagnosticsEngine::Warning,
-                            ("ParmVarDecl, use \"bool\" instead of"
-                             " \"sal_Bool\"%0"),
-                            loc)
-                            << (k == OverrideKind::MAYBE
-                                ? (" (unless this member function overrides a"
-                                   " dependent base member function, even"
-                                   " though it is not marked 'override')")
-                                : "")
-                            << decl->getSourceRange();
-                    }
+                    parmVarDecls_.insert({decl, fbk});
                 }
             }
         }
@@ -600,96 +868,92 @@ bool SalBool::VisitParmVarDecl(ParmVarDecl const * decl) {
     return true;
 }
 
-bool SalBool::WalkUpFromVarDecl(VarDecl const * decl) {
+bool FakeBool::WalkUpFromVarDecl(VarDecl const * decl) {
     return VisitVarDecl(decl);
 }
 
-bool SalBool::VisitVarDecl(VarDecl const * decl) {
+bool FakeBool::VisitVarDecl(VarDecl const * decl) {
     if (ignoreLocation(decl)) {
         return true;
     }
-    if (!decl->isExternC()
-        && (isSalBool(decl->getType()) || isSalBoolArray(decl->getType()))
-        && !isInSpecialMainFile(
-            compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(decl))))
+    if (decl->isExternC()) {
+        return true;
+    }
+    auto k = isFakeBool(decl->getType());
+    if (k == FBK_No) {
+        k = isFakeBoolArray(decl->getType());
+    }
+    if (k == FBK_No) {
+        return true;
+    }
+    auto const loc = compat::getBeginLoc(decl);
+    if (k == FBK_sal_Bool
+        && isInSpecialMainFile(
+            compiler.getSourceManager().getSpellingLoc(loc)))
+    {
+        return true;
+    }
+    auto l = loc;
+    while (compiler.getSourceManager().isMacroArgExpansion(l)) {
+        l = compiler.getSourceManager().getImmediateMacroCallerLoc(l);
+    }
+    if (compiler.getSourceManager().isMacroBodyExpansion(l)
+        && isSharedCAndCppCode(compiler.getSourceManager().getImmediateMacroCallerLoc(l)))
     {
-        varDecls_.insert(decl);
+        return true;
     }
+    varDecls_.insert({decl, k});
     return true;
 }
 
-bool SalBool::WalkUpFromFieldDecl(FieldDecl const * decl) {
+bool FakeBool::WalkUpFromFieldDecl(FieldDecl const * decl) {
     return VisitFieldDecl(decl);
 }
 
-bool SalBool::VisitFieldDecl(FieldDecl const * decl) {
+bool FakeBool::VisitFieldDecl(FieldDecl const * decl) {
     if (ignoreLocation(decl)) {
         return true;
     }
-    if ((isSalBool(decl->getType()) || isSalBoolArray(decl->getType()))
-        && !isInSpecialMainFile(
+    auto k = isFakeBool(decl->getType());
+    if (k == FBK_No) {
+        k = isFakeBoolArray(decl->getType());
+    }
+    if (k == FBK_No) {
+        return true;
+    }
+    if (!isAllRelevantCodeDefined(decl)) {
+        return true;
+    }
+    if (k == FBK_sal_Bool
+        && isInSpecialMainFile(
             compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(decl))))
     {
-        TagDecl const * td = dyn_cast<TagDecl>(decl->getDeclContext());
-        assert(td != nullptr);
-        if (!(((td->isStruct() || td->isUnion()) && td->isExternCContext())
-              || isInUnoIncludeFile(
-                  compiler.getSourceManager().getSpellingLoc(
-                      decl->getLocation()))))
-        {
-            SourceLocation loc { compat::getBeginLoc(decl) };
-            TypeSourceInfo * tsi = decl->getTypeSourceInfo();
-            if (tsi != nullptr) {
-                SourceLocation l {
-                    compiler.getSourceManager().getExpansionLoc(
-                        tsi->getTypeLoc().getBeginLoc()) };
-                SourceLocation end {
-                    compiler.getSourceManager().getExpansionLoc(
-                        tsi->getTypeLoc().getEndLoc()) };
-                assert(l.isFileID() && end.isFileID());
-                if (l == end
-                    || compiler.getSourceManager().isBeforeInTranslationUnit(
-                        l, end))
-                {
-                    for (;;) {
-                        unsigned n = Lexer::MeasureTokenLength(
-                            l, compiler.getSourceManager(),
-                            compiler.getLangOpts());
-                        std::string s {
-                            compiler.getSourceManager().getCharacterData(l),
-                                n };
-                        if (s == "sal_Bool") {
-                            loc = l;
-                            break;
-                        }
-                        if (l == end) {
-                            break;
-                        }
-                        l = l.getLocWithOffset(std::max<unsigned>(n, 1));
-                    }
-                }
-            }
-            if (!rewrite(loc)) {
-                report(
-                    DiagnosticsEngine::Warning,
-                    "FieldDecl, use \"bool\" instead of \"sal_Bool\"", loc)
-                    << decl->getSourceRange();
-            }
-        }
+        return true;
+    }
+    TagDecl const * td = dyn_cast<TagDecl>(decl->getDeclContext());
+    assert(td != nullptr);
+    if (!(((td->isStruct() || td->isUnion()) && td->isExternCContext())
+          || isInUnoIncludeFile(
+              compiler.getSourceManager().getSpellingLoc(
+                  decl->getLocation()))))
+    {
+        fieldDecls_.insert({decl, k});
     }
     return true;
 }
 
-bool SalBool::WalkUpFromFunctionDecl(FunctionDecl const * decl) {
+bool FakeBool::WalkUpFromFunctionDecl(FunctionDecl const * decl) {
     return VisitFunctionDecl(decl);
 }
 
-bool SalBool::VisitFunctionDecl(FunctionDecl const * decl) {
+bool FakeBool::VisitFunctionDecl(FunctionDecl const * decl) {
     if (ignoreLocation(decl)) {
         return true;
     }
-    if (isSalBool(decl->getReturnType().getNonReferenceType())
-        && !(decl->isDeletedAsWritten() && isa<CXXConversionDecl>(decl)))
+    auto const fbk = isFakeBool(decl->getReturnType().getNonReferenceType());
+    if (fbk != FBK_No
+        && !(decl->isDeletedAsWritten() && isa<CXXConversionDecl>(decl))
+        && isAllRelevantCodeDefined(decl))
     {
         FunctionDecl const * f = decl->getCanonicalDecl();
         OverrideKind k = getOverrideKind(f);
@@ -698,68 +962,28 @@ bool SalBool::VisitFunctionDecl(FunctionDecl const * decl) {
                  || (isInUnoIncludeFile(f)
                      && (!f->isInlined() || f->hasAttr<DeprecatedAttr>()))))
         {
-            SourceLocation loc { compat::getBeginLoc(decl) };
-            SourceLocation l { compiler.getSourceManager().getExpansionLoc(
-                loc) };
-            SourceLocation end { compiler.getSourceManager().getExpansionLoc(
-                decl->getNameInfo().getLoc()) };
-            assert(l.isFileID() && end.isFileID());
-            if (compiler.getSourceManager().isBeforeInTranslationUnit(l, end)) {
-                while (l != end) {
-                    unsigned n = Lexer::MeasureTokenLength(
-                        l, compiler.getSourceManager(), compiler.getLangOpts());
-                    std::string s {
-                        compiler.getSourceManager().getCharacterData(l), n };
-                    if (s == "sal_Bool") {
-                        loc = l;
-                        break;
-                    }
-                    l = l.getLocWithOffset(std::max<unsigned>(n, 1));
-                }
-            }
-            // Only rewrite declarations in include files if a definition is
-            // also seen, to avoid compilation of a definition (in a main file
-            // only processed later) to fail with a "mismatch" error before the
-            // rewriter had a chance to act upon the definition (but use the
-            // heuristic of assuming pure virtual functions do not have
-            // definitions):
-            if (!((compiler.getSourceManager().isInMainFile(
-                       compiler.getSourceManager().getSpellingLoc(
-                           decl->getNameInfo().getLoc()))
-                   || f->isDefined() || f->isPure())
-                  && rewrite(loc)))
-            {
-                report(
-                    DiagnosticsEngine::Warning,
-                    "use \"bool\" instead of \"sal_Bool\" as return type%0",
-                    loc)
-                    << (k == OverrideKind::MAYBE
-                        ? (" (unless this member function overrides a dependent"
-                           " base member function, even though it is not marked"
-                           " 'override')")
-                        : "")
-                    << decl->getSourceRange();
-            }
+            functionDecls_.insert({decl, fbk});
         }
     }
     return true;
 }
 
-bool SalBool::VisitValueDecl(ValueDecl const * decl) {
+bool FakeBool::VisitValueDecl(ValueDecl const * decl) {
     if (ignoreLocation(decl)) {
         return true;
     }
-    if (isSalBool(decl->getType()) && !rewrite(compat::getBeginLoc(decl))) {
+    auto const k = isFakeBool(decl->getType());
+    if (k != FBK_No && !rewrite(compat::getBeginLoc(decl), k)) {
         report(
             DiagnosticsEngine::Warning,
-            "ValueDecl, use \"bool\" instead of \"sal_Bool\"",
+            "ValueDecl, use \"bool\" instead of %0",
             compat::getBeginLoc(decl))
-            << decl->getSourceRange();
+            << decl->getType() << decl->getSourceRange();
     }
     return true;
 }
 
-bool SalBool::TraverseStaticAssertDecl(StaticAssertDecl * decl) {
+bool FakeBool::TraverseStaticAssertDecl(StaticAssertDecl * decl) {
     // Ignore special code like
     //
     //   static_cast<sal_Bool>(true) == sal_True
@@ -772,7 +996,7 @@ bool SalBool::TraverseStaticAssertDecl(StaticAssertDecl * decl) {
         || RecursiveASTVisitor::TraverseStaticAssertDecl(decl);
 }
 
-bool SalBool::TraverseLinkageSpecDecl(LinkageSpecDecl * decl) {
+bool FakeBool::TraverseLinkageSpecDecl(LinkageSpecDecl * decl) {
     assert(externCContexts_ != std::numeric_limits<unsigned int>::max()); //TODO
     ++externCContexts_;
     bool ret = RecursiveASTVisitor::TraverseLinkageSpecDecl(decl);
@@ -781,7 +1005,7 @@ bool SalBool::TraverseLinkageSpecDecl(LinkageSpecDecl * decl) {
     return ret;
 }
 
-bool SalBool::isFromCIncludeFile(SourceLocation spellingLocation) const {
+bool FakeBool::isFromCIncludeFile(SourceLocation spellingLocation) const {
     return !compiler.getSourceManager().isInMainFile(spellingLocation)
         && (StringRef(
                 compiler.getSourceManager().getPresumedLoc(spellingLocation)
@@ -789,7 +1013,7 @@ bool SalBool::isFromCIncludeFile(SourceLocation spellingLocation) const {
             .endswith(".h"));
 }
 
-bool SalBool::isSharedCAndCppCode(SourceLocation location) const {
+bool FakeBool::isSharedCAndCppCode(SourceLocation location) const {
     // Assume that code is intended to be shared between C and C++ if it comes
     // from an include file ending in .h, and is either in an extern "C" context
     // or the body of a macro definition:
@@ -799,7 +1023,7 @@ bool SalBool::isSharedCAndCppCode(SourceLocation location) const {
             || compiler.getSourceManager().isMacroBodyExpansion(location));
 }
 
-bool SalBool::isInSpecialMainFile(SourceLocation spellingLocation) const {
+bool FakeBool::isInSpecialMainFile(SourceLocation spellingLocation) const {
     if (!compiler.getSourceManager().isInMainFile(spellingLocation)) {
         return false;
     }
@@ -809,7 +1033,7 @@ bool SalBool::isInSpecialMainFile(SourceLocation spellingLocation) const {
         // TODO: the offset checks
 }
 
-bool SalBool::rewrite(SourceLocation location) {
+bool FakeBool::rewrite(SourceLocation location, FakeBoolKind kind) {
     if (rewriter != nullptr) {
         //TODO: "::sal_Bool" -> "bool", not "::bool"
         SourceLocation loc { compiler.getSourceManager().getExpansionLoc(
@@ -817,7 +1041,7 @@ bool SalBool::rewrite(SourceLocation location) {
         unsigned n = Lexer::MeasureTokenLength(
             loc, compiler.getSourceManager(), compiler.getLangOpts());
         if (std::string(compiler.getSourceManager().getCharacterData(loc), n)
-            == "sal_Bool")
+            == getName(kind))
         {
             return replaceText(loc, n, "bool");
         }
@@ -825,7 +1049,7 @@ bool SalBool::rewrite(SourceLocation location) {
     return false;
 }
 
-loplugin::Plugin::Registration<SalBool> X("salbool", true);
+loplugin::Plugin::Registration<FakeBool> X("fakebool", true);
 
 }
 
diff --git a/compilerplugins/clang/test/salbool.cxx b/compilerplugins/clang/test/fakebool.cxx
index da861afe73be..26b5d7e2f791 100644
--- a/compilerplugins/clang/test/salbool.cxx
+++ b/compilerplugins/clang/test/fakebool.cxx
@@ -11,8 +11,12 @@
 
 #include <sal/types.h>
 
+namespace {
+
 struct S {
-    sal_Bool b; // expected-error {{FieldDecl, use "bool" instead of "sal_Bool" [loplugin:salbool]}}
+    sal_Bool b; // expected-error {{FieldDecl, use "bool" instead of 'sal_Bool' (aka 'unsigned char') [loplugin:fakebool]}}
 };
 
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */