[Clang] [NFC] Move diagnostics emitting code from DiagnosticIDs into DiagnosticsEngine

[Sirraide]

It makes more sense for this functionality to be all in one place rather than split up across two files—at least it caused me a bit of a headache to try and find all places where we were actually forwarding the diagnostic to the DiagnosticConsumer. Moreover, moving these functions into DiagnosticsEngine simplifies the code quite a bit since we access members of DiagnosticsEngine more frequently than those of DiagnosticIDs. There was also a duplicated code snippet that I’ve moved out into a new function.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

It makes more sense for this functionality to be all in one place rather than split up across two files—at least it caused me a bit of a headache to try and find all places where we were actually forwarding the diagnostic to the DiagnosticConsumer. Moreover, moving these functions into DiagnosticsEngine simplifies the code quite a bit since we access members of DiagnosticsEngine more frequently than those of DiagnosticIDs. There was also a duplicated code snippet that I’ve moved out into a new function.

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/Diagnostic.h (+13-10)
• (modified) clang/include/clang/Basic/DiagnosticIDs.h (-12)
• (modified) clang/lib/Basic/Diagnostic.cpp (+91-9)
• (modified) clang/lib/Basic/DiagnosticIDs.cpp (-97)

diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index e9c54c3c487c9..7696acfb45f92 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -18,6 +18,7 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
+#include "clang/Basic/UnsignedOrNone.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FunctionExtras.h"
@@ -49,6 +50,7 @@ class FileSystem;
 namespace clang {
 
 class DeclContext;
+class Diagnostic;
 class DiagnosticBuilder;
 class DiagnosticConsumer;
 class IdentifierInfo;
@@ -228,6 +230,8 @@ class DiagStorageAllocator {
 class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
 public:
   /// The level of the diagnostic, after it has been through mapping.
+  // FIXME: Make this an alias for DiagnosticIDs::Level as soon as
+  // we can use 'using enum'.
   enum Level {
     Ignored = DiagnosticIDs::Ignored,
     Note = DiagnosticIDs::Note,
@@ -532,7 +536,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   ///
   /// This is used to emit continuation diagnostics with the same level as the
   /// diagnostic that they follow.
-  DiagnosticIDs::Level LastDiagLevel;
+  Level LastDiagLevel;
 
   /// Number of warnings reported
   unsigned NumWarnings;
@@ -777,18 +781,16 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// the middle of another diagnostic.
   ///
   /// This can be used by clients who suppress diagnostics themselves.
-  void setLastDiagnosticIgnored(bool Ignored) {
-    if (LastDiagLevel == DiagnosticIDs::Fatal)
+  void setLastDiagnosticIgnored(bool IsIgnored) {
+    if (LastDiagLevel == Fatal)
       FatalErrorOccurred = true;
-    LastDiagLevel = Ignored ? DiagnosticIDs::Ignored : DiagnosticIDs::Warning;
+    LastDiagLevel = IsIgnored ? Ignored : Warning;
   }
 
   /// Determine whether the previous diagnostic was ignored. This can
   /// be used by clients that want to determine whether notes attached to a
   /// diagnostic will be suppressed.
-  bool isLastDiagnosticIgnored() const {
-    return LastDiagLevel == DiagnosticIDs::Ignored;
-  }
+  bool isLastDiagnosticIgnored() const { return LastDiagLevel == Ignored; }
 
   /// Controls whether otherwise-unmapped extension diagnostics are
   /// mapped onto ignore/warning/error.
@@ -1024,9 +1026,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// Used to report a diagnostic that is finally fully formed.
   ///
   /// \returns true if the diagnostic was emitted, false if it was suppressed.
-  bool ProcessDiag(const DiagnosticBuilder &DiagBuilder) {
-    return Diags->ProcessDiag(*this, DiagBuilder);
-  }
+  bool ProcessDiag(const DiagnosticBuilder &DiagBuilder);
 
   /// @name Diagnostic Emission
   /// @{
@@ -1046,6 +1046,9 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   bool EmitDiagnostic(const DiagnosticBuilder &DB, bool Force = false);
 
   /// @}
+
+private:
+  void Report(Level DiagLevel, const Diagnostic &Info);
 };
 
 /// RAII class that determines when any errors have occurred
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index 80d52a0d01112..2b095f0fd6741 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -483,18 +483,6 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 
   Class getDiagClass(unsigned DiagID) const;
 
-  /// Used to report a diagnostic that is finally fully formed.
-  ///
-  /// \returns \c true if the diagnostic was emitted, \c false if it was
-  /// suppressed.
-  bool ProcessDiag(DiagnosticsEngine &Diag,
-                   const DiagnosticBuilder &DiagBuilder) const;
-
-  /// Used to emit a diagnostic that is finally fully formed,
-  /// ignoring suppression.
-  void EmitDiag(DiagnosticsEngine &Diag, const DiagnosticBuilder &DiagBuilder,
-                Level DiagLevel) const;
-
   /// Whether the diagnostic may leave the AST in a state where some
   /// invariants can break.
   bool isUnrecoverable(unsigned DiagID) const;
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 694224071347a..2718874703bfe 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -130,7 +130,7 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) {
   TrapNumErrorsOccurred = 0;
   TrapNumUnrecoverableErrorsOccurred = 0;
 
-  LastDiagLevel = DiagnosticIDs::Ignored;
+  LastDiagLevel = Ignored;
 
   if (!soft) {
     // Clear state related to #pragma diagnostic.
@@ -658,13 +658,97 @@ void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   Level DiagLevel = storedDiag.getLevel();
   Diagnostic Info(this, storedDiag.getLocation(), storedDiag.getID(),
                   DiagStorage, storedDiag.getMessage());
+  Report(DiagLevel, Info);
+}
+
+void DiagnosticsEngine::Report(Level DiagLevel, const Diagnostic &Info) {
+  assert(DiagLevel != Ignored && "Cannot emit ignored diagnostics!");
   Client->HandleDiagnostic(DiagLevel, Info);
   if (Client->IncludeInDiagnosticCounts()) {
-    if (DiagLevel == DiagnosticsEngine::Warning)
+    if (DiagLevel == Warning)
       ++NumWarnings;
   }
 }
 
+/// ProcessDiag - This is the method used to report a diagnostic that is
+/// finally fully formed.
+bool DiagnosticsEngine::ProcessDiag(const DiagnosticBuilder &DiagBuilder) {
+  Diagnostic Info(this, DiagBuilder);
+
+  assert(getClient() && "DiagnosticClient not set!");
+
+  // Figure out the diagnostic level of this message.
+  unsigned DiagID = Info.getID();
+  Level DiagLevel = getDiagnosticLevel(DiagID, Info.getLocation());
+
+  // Update counts for DiagnosticErrorTrap even if a fatal error occurred
+  // or diagnostics are suppressed.
+  if (DiagLevel >= Error) {
+    ++TrapNumErrorsOccurred;
+    if (Diags->isUnrecoverable(DiagID))
+      ++TrapNumUnrecoverableErrorsOccurred;
+  }
+
+  if (SuppressAllDiagnostics)
+    return false;
+
+  if (DiagLevel != Note) {
+    // Record that a fatal error occurred only when we see a second
+    // non-note diagnostic. This allows notes to be attached to the
+    // fatal error, but suppresses any diagnostics that follow those
+    // notes.
+    if (LastDiagLevel == Fatal)
+      FatalErrorOccurred = true;
+
+    LastDiagLevel = DiagLevel;
+  }
+
+  // If a fatal error has already been emitted, silence all subsequent
+  // diagnostics.
+  if (FatalErrorOccurred) {
+    if (DiagLevel >= Error && Client->IncludeInDiagnosticCounts()) {
+      ++NumErrors;
+    }
+
+    return false;
+  }
+
+  // If the client doesn't care about this message, don't issue it.  If this is
+  // a note and the last real diagnostic was ignored, ignore it too.
+  if (DiagLevel == Ignored || (DiagLevel == Note && LastDiagLevel == Ignored))
+    return false;
+
+  if (DiagLevel >= Error) {
+    if (Diags->isUnrecoverable(DiagID))
+      UnrecoverableErrorOccurred = true;
+
+    // Warnings which have been upgraded to errors do not prevent compilation.
+    if (Diags->isDefaultMappingAsError(DiagID))
+      UncompilableErrorOccurred = true;
+
+    ErrorOccurred = true;
+    if (Client->IncludeInDiagnosticCounts()) {
+      ++NumErrors;
+    }
+
+    // If we've emitted a lot of errors, emit a fatal error instead of it to
+    // stop a flood of bogus errors.
+    if (ErrorLimit && NumErrors > ErrorLimit && DiagLevel == Error) {
+      Report(diag::fatal_too_many_errors);
+      return false;
+    }
+  }
+
+  // Make sure we set FatalErrorOccurred to ensure that the notes from the
+  // diagnostic that caused `fatal_too_many_errors` won't be emitted.
+  if (Info.getID() == diag::fatal_too_many_errors)
+    FatalErrorOccurred = true;
+
+  // Finally, report it.
+  Report(DiagLevel, Info);
+  return true;
+}
+
 bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB,
                                        bool Force) {
   assert(getClient() && "DiagnosticClient not set!");
@@ -674,14 +758,12 @@ bool DiagnosticsEngine::EmitDiagnostic(const DiagnosticBuilder &DB,
     Diagnostic Info(this, DB);
 
     // Figure out the diagnostic level of this message.
-    DiagnosticIDs::Level DiagLevel =
-        Diags->getDiagnosticLevel(Info.getID(), Info.getLocation(), *this);
+    Level DiagLevel = getDiagnosticLevel(Info.getID(), Info.getLocation());
 
-    Emitted = (DiagLevel != DiagnosticIDs::Ignored);
-    if (Emitted) {
-      // Emit the diagnostic regardless of suppression level.
-      Diags->EmitDiag(*this, DB, DiagLevel);
-    }
+    // Emit the diagnostic regardless of suppression level.
+    Emitted = DiagLevel != Ignored;
+    if (Emitted)
+      Report(DiagLevel, Info);
   } else {
     // Process the diagnostic, sending the accumulated information to the
     // DiagnosticConsumer.
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index 3e90b2d804773..dcf0c6cb54282 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -823,103 +823,6 @@ unsigned DiagnosticIDs::getCXXCompatDiagId(const LangOptions &LangOpts,
   return StdVer >= D.StdVer ? D.DiagId : D.PreDiagId;
 }
 
-/// ProcessDiag - This is the method used to report a diagnostic that is
-/// finally fully formed.
-bool DiagnosticIDs::ProcessDiag(DiagnosticsEngine &Diag,
-                                const DiagnosticBuilder &DiagBuilder) const {
-  Diagnostic Info(&Diag, DiagBuilder);
-
-  assert(Diag.getClient() && "DiagnosticClient not set!");
-
-  // Figure out the diagnostic level of this message.
-  unsigned DiagID = Info.getID();
-  DiagnosticIDs::Level DiagLevel
-    = getDiagnosticLevel(DiagID, Info.getLocation(), Diag);
-
-  // Update counts for DiagnosticErrorTrap even if a fatal error occurred
-  // or diagnostics are suppressed.
-  if (DiagLevel >= DiagnosticIDs::Error) {
-    ++Diag.TrapNumErrorsOccurred;
-    if (isUnrecoverable(DiagID))
-      ++Diag.TrapNumUnrecoverableErrorsOccurred;
-  }
-
-  if (Diag.SuppressAllDiagnostics)
-    return false;
-
-  if (DiagLevel != DiagnosticIDs::Note) {
-    // Record that a fatal error occurred only when we see a second
-    // non-note diagnostic. This allows notes to be attached to the
-    // fatal error, but suppresses any diagnostics that follow those
-    // notes.
-    if (Diag.LastDiagLevel == DiagnosticIDs::Fatal)
-      Diag.FatalErrorOccurred = true;
-
-    Diag.LastDiagLevel = DiagLevel;
-  }
-
-  // If a fatal error has already been emitted, silence all subsequent
-  // diagnostics.
-  if (Diag.FatalErrorOccurred) {
-    if (DiagLevel >= DiagnosticIDs::Error &&
-        Diag.Client->IncludeInDiagnosticCounts()) {
-      ++Diag.NumErrors;
-    }
-
-    return false;
-  }
-
-  // If the client doesn't care about this message, don't issue it.  If this is
-  // a note and the last real diagnostic was ignored, ignore it too.
-  if (DiagLevel == DiagnosticIDs::Ignored ||
-      (DiagLevel == DiagnosticIDs::Note &&
-       Diag.LastDiagLevel == DiagnosticIDs::Ignored))
-    return false;
-
-  if (DiagLevel >= DiagnosticIDs::Error) {
-    if (isUnrecoverable(DiagID))
-      Diag.UnrecoverableErrorOccurred = true;
-
-    // Warnings which have been upgraded to errors do not prevent compilation.
-    if (isDefaultMappingAsError(DiagID))
-      Diag.UncompilableErrorOccurred = true;
-
-    Diag.ErrorOccurred = true;
-    if (Diag.Client->IncludeInDiagnosticCounts()) {
-      ++Diag.NumErrors;
-    }
-
-    // If we've emitted a lot of errors, emit a fatal error instead of it to
-    // stop a flood of bogus errors.
-    if (Diag.ErrorLimit && Diag.NumErrors > Diag.ErrorLimit &&
-        DiagLevel == DiagnosticIDs::Error) {
-      Diag.Report(diag::fatal_too_many_errors);
-      return false;
-    }
-  }
-
-  // Make sure we set FatalErrorOccurred to ensure that the notes from the
-  // diagnostic that caused `fatal_too_many_errors` won't be emitted.
-  if (Info.getID() == diag::fatal_too_many_errors)
-    Diag.FatalErrorOccurred = true;
-  // Finally, report it.
-  EmitDiag(Diag, DiagBuilder, DiagLevel);
-  return true;
-}
-
-void DiagnosticIDs::EmitDiag(DiagnosticsEngine &Diag,
-                             const DiagnosticBuilder &DiagBuilder,
-                             Level DiagLevel) const {
-  Diagnostic Info(&Diag, DiagBuilder);
-  assert(DiagLevel != DiagnosticIDs::Ignored && "Cannot emit ignored diagnostics!");
-
-  Diag.Client->HandleDiagnostic((DiagnosticsEngine::Level)DiagLevel, Info);
-  if (Diag.Client->IncludeInDiagnosticCounts()) {
-    if (DiagLevel == DiagnosticIDs::Warning)
-      ++Diag.NumWarnings;
-  }
-}
-
 bool DiagnosticIDs::isUnrecoverable(unsigned DiagID) const {
   // Only errors may be unrecoverable.
   if (getDiagClass(DiagID) < CLASS_ERROR)

[Fznamznon]

That makes sense to me, but I've never touched that before, so probably makes sense to wait for the feedback from @AaronBallman too.

[AaronBallman]

LGTM!

Linux precommit CI failures appear to be unrelated:

/usr/bin/ld: cannot find /home/gha/actions-runner/_work/llvm-project/llvm-project/build/lib/clang/21/lib/x86_64-unknown-linux-gnu/libclang_rt.tysan.a: No such file or directory