Skip to content

Conversation

@delcypher
Copy link
Contributor

@delcypher delcypher commented Aug 20, 2025

When trying to add a new diagnostic category (e.g. #154618) I discovered
clang-format really wanted to reformat these files.

My initial attempt was just to suppress the reformatting with // clang-format (on|off) directives but reviewers preferred just
reformatting the files so these two files have been completely
reformatted.

clang-format has been disabled for the enum that declares the
DIAG_START_* constants because its much less readable after
formatting.

@delcypher delcypher self-assigned this Aug 20, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 20, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 20, 2025

@llvm/pr-subscribers-clang

Author: Dan Liew (delcypher)

Changes

When trying to add a new diagnostic category (e.g. #154618), clang-format tries to reformat parts of AllDiagnostics.h and DiagnosticsIDs.h and messes it up. This is unhelpful and is especially unhelpful now that we have a CI job that checks that PRs are correctly formatted.

Fix this by disabling clang-format in these parts.


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/AllDiagnostics.h (+2)
  • (modified) clang/include/clang/Basic/DiagnosticIDs.h (+2)
diff --git a/clang/include/clang/Basic/AllDiagnostics.h b/clang/include/clang/Basic/AllDiagnostics.h
index e64634cc138f7..340f7d5a4d57f 100644
--- a/clang/include/clang/Basic/AllDiagnostics.h
+++ b/clang/include/clang/Basic/AllDiagnostics.h
@@ -14,6 +14,7 @@
 #ifndef LLVM_CLANG_BASIC_ALLDIAGNOSTICS_H
 #define LLVM_CLANG_BASIC_ALLDIAGNOSTICS_H
 
+// clang-format off
 #include "clang/Basic/DiagnosticAST.h"
 #include "clang/Basic/DiagnosticAnalysis.h"
 #include "clang/Basic/DiagnosticComment.h"
@@ -26,6 +27,7 @@
 #include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/DiagnosticSerialization.h"
 #include "clang/Basic/DiagnosticRefactoring.h"
+// clang-format on
 
 namespace clang {
 template <size_t SizeOfStr, typename FieldType>
diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h
index b21a3b6232fc0..0731afc490fd8 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -32,6 +32,7 @@ namespace clang {
   namespace diag {
     enum class Group;
 
+    // clang-format off
     // Size of each of the diagnostic categories.
     enum {
       DIAG_SIZE_COMMON        =  300,
@@ -65,6 +66,7 @@ namespace clang {
       DIAG_START_INSTALLAPI    = DIAG_START_REFACTORING   + static_cast<int>(DIAG_SIZE_REFACTORING),
       DIAG_UPPER_LIMIT         = DIAG_START_INSTALLAPI    + static_cast<int>(DIAG_SIZE_INSTALLAPI)
     };
+    // clang-format on
 
     class CustomDiagInfo;
 

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What formatting is it doing here? We very much LIKE our include file reordering/formatting, so I wonder what it is messing up here? Can you share the format-diff for me?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what is suggested here: https://github.com/llvm/llvm-project/actions/runs/17110487879/job/48530463722?pr=154618

What is wrong with its sugestion? It looks right to me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo the ‘fix’ for this is to just reformat this section of the file because the formatting is... all sorts of wrong here to begin with (e.g. the namespace contents are indented), so no wonder clang-format would keep trying to reformat this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had assumed that the inclusion order was deliberate but taking a closer look I see it doesn't actually match the declaration order of the categories (i.e. Common, Driver, Frontend`...). I'll just reformat this bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirraide Is it both files you were thinking of reformatting? I'm happy to reformat AllDiagnostics.h but clang-format makes a real mess of DiagnosticIDs.h. The declaration of the enum is very deliberately formatted to be readable and isn't as readable after formatting which looks like:

    enum {
      DIAG_START_COMMON = 0, 
      DIAG_START_DRIVER =
          DIAG_START_COMMON + static_cast<int>(DIAG_SIZE_COMMON),
      DIAG_START_FRONTEND =
          DIAG_START_DRIVER + static_cast<int>(DIAG_SIZE_DRIVER),
      DIAG_START_SERIALIZATION =
          DIAG_START_FRONTEND + static_cast<int>(DIAG_SIZE_FRONTEND),
      DIAG_START_LEX =
          DIAG_START_SERIALIZATION + static_cast<int>(DIAG_SIZE_SERIALIZATION),
      DIAG_START_PARSE = DIAG_START_LEX + static_cast<int>(DIAG_SIZE_LEX),
      DIAG_START_AST = DIAG_START_PARSE + static_cast<int>(DIAG_SIZE_PARSE),
      DIAG_START_COMMENT = DIAG_START_AST + static_cast<int>(DIAG_SIZE_AST),
      DIAG_START_CROSSTU =
          DIAG_START_COMMENT + static_cast<int>(DIAG_SIZE_COMMENT),
      DIAG_START_SEMA =
          DIAG_START_CROSSTU + static_cast<int>(DIAG_SIZE_CROSSTU),
      DIAG_START_ANALYSIS = DIAG_START_SEMA + static_cast<int>(DIAG_SIZE_SEMA),
      DIAG_START_REFACTORING =
          DIAG_START_ANALYSIS + static_cast<int>(DIAG_SIZE_ANALYSIS),
      DIAG_START_INSTALLAPI =
          DIAG_START_REFACTORING + static_cast<int>(DIAG_SIZE_REFACTORING),
      DIAG_UPPER_LIMIT =
          DIAG_START_INSTALLAPI + static_cast<int>(DIAG_SIZE_INSTALLAPI)
    };

whereas before it was:

    enum {
      DIAG_START_COMMON        =                          0,
      DIAG_START_DRIVER        = DIAG_START_COMMON        + static_cast<int>(DIAG_SIZE_COMMON),
      DIAG_START_FRONTEND      = DIAG_START_DRIVER        + static_cast<int>(DIAG_SIZE_DRIVER),
      DIAG_START_SERIALIZATION = DIAG_START_FRONTEND      + static_cast<int>(DIAG_SIZE_FRONTEND),
      DIAG_START_LEX           = DIAG_START_SERIALIZATION + static_cast<int>(DIAG_SIZE_SERIALIZATION),
      DIAG_START_PARSE         = DIAG_START_LEX           + static_cast<int>(DIAG_SIZE_LEX),
      DIAG_START_AST           = DIAG_START_PARSE         + static_cast<int>(DIAG_SIZE_PARSE),
      DIAG_START_COMMENT       = DIAG_START_AST           + static_cast<int>(DIAG_SIZE_AST),
      DIAG_START_CROSSTU       = DIAG_START_COMMENT       + static_cast<int>(DIAG_SIZE_COMMENT),
      DIAG_START_SEMA          = DIAG_START_CROSSTU       + static_cast<int>(DIAG_SIZE_CROSSTU),
      DIAG_START_ANALYSIS      = DIAG_START_SEMA          + static_cast<int>(DIAG_SIZE_SEMA),
      DIAG_START_REFACTORING   = DIAG_START_ANALYSIS      + static_cast<int>(DIAG_SIZE_ANALYSIS),
      DIAG_START_INSTALLAPI    = DIAG_START_REFACTORING   + static_cast<int>(DIAG_SIZE_REFACTORING),
      DIAG_UPPER_LIMIT         = DIAG_START_INSTALLAPI    + static_cast<int>(DIAG_SIZE_INSTALLAPI)
    };

@delcypher delcypher force-pushed the dliew/disable-clang-format-diag branch from 866ddbb to 62eccfc Compare August 21, 2025 17:51
When trying to add a new diagnostic category (e.g. llvm#154618) I discovered
`clang-format` really wanted to reformat these files.

My initial attempt was just to suppress the reformatting with `//
clang-format (on|off)` directives but reviewers preferred just
reformatting the files so these two files have been completely
reformatted.

`clang-format` has been disabled for the enum that declares the
`DIAG_START_*` constants because its much less readable after
formatting.
@delcypher delcypher force-pushed the dliew/disable-clang-format-diag branch from 62eccfc to 7c8a131 Compare August 21, 2025 17:52
@delcypher
Copy link
Contributor Author

@erichkeane @Sirraide I just reformatted the entire files (opting out the enum that declares the DIAG_START_* constants for readability. Assuming CI passes, is this good to go?

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, LGTM.

@delcypher delcypher changed the title [NFC][Diagnostics] Prevent incorrect clang-format reformatting in some headers related to Clang diagnostics [NFC][Diagnostics] Reformat DiagnosticIDs.h and AllDiagnostics.h Aug 21, 2025
@delcypher
Copy link
Contributor Author

The CI failure is due to an unrelated test failure.

  ********************
  Unresolved Tests (1):
    lldb-api :: tools/lldb-dap/moduleSymbols/TestDAP_moduleSymbols.py

@delcypher delcypher merged commit 0961200 into llvm:main Aug 21, 2025
8 of 9 checks passed
delcypher added a commit to swiftlang/llvm-project that referenced this pull request Sep 9, 2025
…lvm#154628)

When trying to add a new diagnostic category (e.g. llvm#154618) I discovered
`clang-format` really wanted to reformat these files.

My initial attempt was just to suppress the reformatting with `//
clang-format (on|off)` directives but reviewers preferred just
reformatting the files so these two files have been completely
reformatted.

`clang-format` has been disabled for the enum that declares the
`DIAG_START_*` constants because its much less readable after
formatting.

(cherry picked from commit 0961200)

Conflicts:
	clang/include/clang/Basic/DiagnosticIDs.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants