Skip to content

[diagtool] Make the BuiltinDiagnosticsByID table sorted #120321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 17, 2025
Merged

[diagtool] Make the BuiltinDiagnosticsByID table sorted #120321

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ad62a9
[diagtool] Make the BuiltinDiagnosticsByID table sorted
karka228 Dec 17, 2024
4cedc90
[diagtool] Make the BuiltinDiagnosticsByID table sorted
karka228 Jan 7, 2025
81743f8
[diagtool] Make the BuiltinDiagnosticsByID table sorted
karka228 Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions clang/tools/diagtool/DiagnosticNames.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,29 @@ llvm::ArrayRef<DiagnosticRecord> diagtool::getBuiltinDiagnosticsByName() {
return llvm::ArrayRef(BuiltinDiagnosticsByName);
}


// FIXME: Is it worth having two tables, especially when this one can get
// out of sync easily?
// clang-format off
Copy link
Collaborator Author

@karka228 karka228 Dec 29, 2024

Choose a reason for hiding this comment

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

Add a comment about why we turn off clang-format:

// Turn off clang-format, as the order of the includes are important
// to make sure the table is sorted.

Copy link
Member

@kadircet kadircet Jan 3, 2025

Choose a reason for hiding this comment

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

also it's unclear where the "ground truth" for this order is coming from. AFAICT, order is defined by https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticIDs.h#L49-L65
and enforced by very subtle interactions between tablegen and how particular headers include these.

since this order needs to be preserved in multiple places, can you put together a wrapper header, which includes individual diagnostic kinds in the specific order and explain the relationship between these pieces? (I'd rather have a solution that puts all of this logic into tablegen instead, generating enums with a particular order once, but I can see that's a much bigger change that you might not want to sign up).

Copy link
Collaborator Author

@karka228 karka228 Jan 16, 2025

Choose a reason for hiding this comment

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

"since this order needs to be preserved in multiple places, can you put together a wrapper header, which includes individual diagnostic kinds in the specific order and explain the relationship between these pieces?"
@kadircet I added a new file DiagnosticsID.inc as a "wrapper header".

static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR, \
SHOWINSYSHEADER, SHOWINSYSMACRO, DEFER, CATEGORY) \
{#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
#include "clang/Basic/DiagnosticCommonKinds.inc"
#include "clang/Basic/DiagnosticCrossTUKinds.inc"
#include "clang/Basic/DiagnosticDriverKinds.inc"
#include "clang/Basic/DiagnosticFrontendKinds.inc"
#include "clang/Basic/DiagnosticSerializationKinds.inc"
#include "clang/Basic/DiagnosticLexKinds.inc"
#include "clang/Basic/DiagnosticParseKinds.inc"
#include "clang/Basic/DiagnosticASTKinds.inc"
#include "clang/Basic/DiagnosticCommentKinds.inc"
#include "clang/Basic/DiagnosticCrossTUKinds.inc"
Copy link
Collaborator Author

@karka228 karka228 Dec 29, 2024

Choose a reason for hiding this comment

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

Verify the changed include order by adding an assert in getDiagnosticForID below:

  // The requirement for lower_bound to produce a valid result it is
  // enough if the BuiltinDiagnosticsByID is partitioned (by DiagID),
  // but as we want this function to work for all possible values of
  // DiagID sent in as argument it is better to right away check if
  // BuiltinDiagnosticsByID is sorted.
  assert(llvm::is_sorted(BuiltinDiagnosticsByID, orderByID) &&
	 "IDs in BuiltinDiagnosticsByID must be sorted.");

Copy link
Contributor

@cor3ntin cor3ntin Dec 30, 2024

Choose a reason for hiding this comment

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

This seems brittle - Maybe we should call std::sort somewhere instead?

Copy link
Collaborator Author

@karka228 karka228 Jan 7, 2025

Choose a reason for hiding this comment

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

This seems brittle - Maybe we should call std::sort somewhere instead?

I agree that it is a bit brittle, but it is hard to find a good place where to insert a std::sort. As @kadircet wrote it is probably better in the future to solve this in a better way in tablegen.

#include "clang/Basic/DiagnosticSemaKinds.inc"
#include "clang/Basic/DiagnosticAnalysisKinds.inc"
#include "clang/Basic/DiagnosticRefactoringKinds.inc"
#include "clang/Basic/DiagnosticInstallAPIKinds.inc"
#undef DIAG
};
// clang-format on

static bool orderByID(const DiagnosticRecord &Left,
const DiagnosticRecord &Right) {
Expand Down
Loading