[clang][NFC] Change suppression mapping interfaces to use SourceLocation

[kadircet]

This way we can delay getting a presumed location even further, only
performing it for diagnostics that are mapped.

[llvmbot]

@llvm/pr-subscribers-clang

Author: kadir çetinkaya (kadircet)

Changes

This way we can delay getting a presumed location even further, only
performing it for diagnostics that are mapped.

---

Full diff: https://github.com/llvm/llvm-project/pull/118960.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/Diagnostic.h (+3-2)
• (modified) clang/lib/Basic/Diagnostic.cpp (+16-7)
• (modified) clang/lib/Basic/DiagnosticIDs.cpp (+2-9)
• (modified) clang/unittests/Basic/DiagnosticTest.cpp (+25-14)

diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index d271accca3d3b7..510b782e35d065 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -560,7 +560,8 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   ArgToStringFnTy ArgToStringFn;
 
   /// Whether the diagnostic should be suppressed in FilePath.
-  llvm::unique_function<bool(diag::kind, StringRef /*FilePath*/) const>
+  llvm::unique_function<bool(diag::kind, SourceLocation /*DiagLoc*/,
+                             const SourceManager &) const>
       DiagSuppressionMapping;
 
 public:
@@ -972,7 +973,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// These take presumed locations into account, and can still be overriden by
   /// clang-diagnostics pragmas.
   void setDiagSuppressionMapping(llvm::MemoryBuffer &Input);
-  bool isSuppressedViaMapping(diag::kind DiagId, StringRef FilePath) const;
+  bool isSuppressedViaMapping(diag::kind DiagId, SourceLocation DiagLoc) const;
 
   /// Issue the message to the client.
   ///
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 2d0e358116362c..682e02222a1993 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -500,7 +500,8 @@ class WarningsSpecialCaseList : public llvm::SpecialCaseList {
   // the last section take precedence in such cases.
   void processSections(DiagnosticsEngine &Diags);
 
-  bool isDiagSuppressed(diag::kind DiagId, StringRef FilePath) const;
+  bool isDiagSuppressed(diag::kind DiagId, SourceLocation DiagLoc,
+                        const SourceManager &SM) const;
 
 private:
   // Find the longest glob pattern that matches FilePath amongst
@@ -573,13 +574,14 @@ void DiagnosticsEngine::setDiagSuppressionMapping(llvm::MemoryBuffer &Input) {
   WarningSuppressionList->processSections(*this);
   DiagSuppressionMapping =
       [WarningSuppressionList(std::move(WarningSuppressionList))](
-          diag::kind DiagId, StringRef Path) {
-        return WarningSuppressionList->isDiagSuppressed(DiagId, Path);
+          diag::kind DiagId, SourceLocation DiagLoc, const SourceManager &SM) {
+        return WarningSuppressionList->isDiagSuppressed(DiagId, DiagLoc, SM);
       };
 }
 
 bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId,
-                                               StringRef FilePath) const {
+                                               SourceLocation DiagLoc,
+                                               const SourceManager &SM) const {
   const Section *DiagSection = DiagToSection.lookup(DiagId);
   if (!DiagSection)
     return false;
@@ -589,7 +591,13 @@ bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId,
     return false;
   const llvm::StringMap<llvm::SpecialCaseList::Matcher> &CategoriesToMatchers =
       SrcEntriesIt->getValue();
-  return globsMatches(CategoriesToMatchers, FilePath);
+  // We also use presumed locations here to improve reproducibility for
+  // preprocessed inputs.
+  if (PresumedLoc PLoc = SM.getPresumedLoc(DiagLoc); PLoc.isValid())
+    return globsMatches(
+        CategoriesToMatchers,
+        llvm::sys::path::remove_leading_dotslash(PLoc.getFilename()));
+  return false;
 }
 
 bool WarningsSpecialCaseList::globsMatches(
@@ -614,8 +622,9 @@ bool WarningsSpecialCaseList::globsMatches(
 }
 
 bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind DiagId,
-                                               StringRef FilePath) const {
-  return DiagSuppressionMapping && DiagSuppressionMapping(DiagId, FilePath);
+                                               SourceLocation DiagLoc) const {
+  if (!hasSourceManager() || !DiagSuppressionMapping) return false;
+  return DiagSuppressionMapping(DiagId, DiagLoc, getSourceManager());
 }
 
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index 44922aa7872dbf..d77f28c80b2eb2 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -601,15 +601,8 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
     return diag::Severity::Ignored;
 
   // Clang-diagnostics pragmas always take precedence over suppression mapping.
-  if (!Mapping.isPragma() && Diag.DiagSuppressionMapping) {
-    // We also use presumed locations here to improve reproducibility for
-    // preprocessed inputs.
-    if (PresumedLoc PLoc = SM.getPresumedLoc(Loc);
-        PLoc.isValid() && Diag.isSuppressedViaMapping(
-                              DiagID, llvm::sys::path::remove_leading_dotslash(
-                                          PLoc.getFilename())))
-      return diag::Severity::Ignored;
-  }
+  if (!Mapping.isPragma() && Diag.isSuppressedViaMapping(DiagID, Loc))
+    return diag::Severity::Ignored;
 
   return Result;
 }
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp
index 36a77c7247655f..5c3d7934b242ec 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -12,6 +12,7 @@
 #include "clang/Basic/DiagnosticLex.h"
 #include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <memory>
 #include <optional>
 #include <vector>
 
@@ -196,7 +198,17 @@ class SuppressionMappingTest : public testing::Test {
     return CaptureConsumer.StoredDiags;
   }
 
+  SourceLocation locForFile(llvm::StringRef FileName) {
+    auto Buf = MemoryBuffer::getMemBuffer("", FileName);
+    SourceManager &SM = Diags.getSourceManager();
+    FileID FooID = SM.createFileID(std::move(Buf));
+    return SM.getLocForStartOfFile(FooID);
+  }
+
 private:
+  FileManager FM{{}, FS};
+  SourceManager SM{Diags, FM};
+
   class CaptureDiagnosticConsumer : public DiagnosticConsumer {
   public:
     std::vector<StoredDiagnostic> StoredDiags;
@@ -255,9 +267,10 @@ TEST_F(SuppressionMappingTest, SuppressesGroup) {
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
   EXPECT_THAT(diags(), IsEmpty());
 
+  SourceLocation FooLoc = locForFile("foo.cpp");
   EXPECT_TRUE(
-      Diags.isSuppressedViaMapping(diag::warn_unused_function, "foo.cpp"));
-  EXPECT_FALSE(Diags.isSuppressedViaMapping(diag::warn_deprecated, "foo.cpp"));
+      Diags.isSuppressedViaMapping(diag::warn_unused_function, FooLoc));
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(diag::warn_deprecated, FooLoc));
 }
 
 TEST_F(SuppressionMappingTest, EmitCategoryIsExcluded) {
@@ -271,10 +284,10 @@ TEST_F(SuppressionMappingTest, EmitCategoryIsExcluded) {
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
   EXPECT_THAT(diags(), IsEmpty());
 
-  EXPECT_TRUE(
-      Diags.isSuppressedViaMapping(diag::warn_unused_function, "bar.cpp"));
-  EXPECT_FALSE(
-      Diags.isSuppressedViaMapping(diag::warn_unused_function, "foo.cpp"));
+  EXPECT_TRUE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
+                                           locForFile("bar.cpp")));
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
+                                            locForFile("foo.cpp")));
 }
 
 TEST_F(SuppressionMappingTest, LongestMatchWins) {
@@ -289,12 +302,12 @@ TEST_F(SuppressionMappingTest, LongestMatchWins) {
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
   EXPECT_THAT(diags(), IsEmpty());
 
+  EXPECT_TRUE(Diags.isSuppressedViaMapping(
+      diag::warn_unused_function, locForFile("clang/lib/Basic/foo.h")));
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(
+      diag::warn_unused_function, locForFile("clang/lib/Sema/bar.h")));
   EXPECT_TRUE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
-                                           "clang/lib/Basic/foo.h"));
-  EXPECT_FALSE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
-                                            "clang/lib/Sema/bar.h"));
-  EXPECT_TRUE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
-                                           "clang/lib/Sema/foo.h"));
+                                           locForFile("clang/lib/Sema/foo.h")));
 }
 
 TEST_F(SuppressionMappingTest, IsIgnored) {
@@ -308,9 +321,7 @@ TEST_F(SuppressionMappingTest, IsIgnored) {
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
   ASSERT_THAT(diags(), IsEmpty());
 
-  FileManager FM({}, FS);
-  SourceManager SM(Diags, FM);
-
+  SourceManager &SM = Diags.getSourceManager();
   auto ClangID =
       SM.createFileID(llvm::MemoryBuffer::getMemBuffer("", "clang/foo.h"));
   auto NonClangID =

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.