Skip to content
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.

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

Audit/Update select uses in DiagnosticXKinds.td to use enum_select. #123121

Open
erichkeane opened this issue Jan 15, 2025 · 9 comments
Open

Audit/Update select uses in DiagnosticXKinds.td to use enum_select. #123121

erichkeane opened this issue Jan 15, 2025 · 9 comments
Assignees
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer good first issue https://github.com/llvm/llvm-project/contribute quality-of-implementation

Comments

@erichkeane
Copy link
Collaborator

enum_select was added in this patch here: #122505

It is a really useful version of select that also creates an enum so that we don't have to use unreliable 'magic numbers'. However, the original patch touches one such diagnostic.

We need someone to audit all of our uses of select and see if:
1- the select has a significant number of items in it.
2- A lot of the uses of the select for the diagnostic in the source use a lot of magic numbers, OR an enum-made-just-for it

So if the diagnostic tends to have a lot of /* ThingIWantSelected */ 5 sort of things (or worse, just the 5!), it is likely a good candidate. Ones that are 'calculated' based on some other criteria aren't a great.

One such example is from: #120327 , which is what encouraged this patch.

Once we have ones identified, someone should then go through and convert these diagnostics.

@erichkeane erichkeane added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer good first issue https://github.com/llvm/llvm-project/contribute labels Jan 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

@llvm/issue-subscribers-good-first-issue

Author: Erich Keane (erichkeane)

`enum_select` was added in this patch here: https://github.com//pull/122505

It is a really useful version of select that also creates an enum so that we don't have to use unreliable 'magic numbers'. However, the original patch touches one such diagnostic.

We need someone to audit all of our uses of select and see if:
1- the select has a significant number of items in it.
2- A lot of the uses of the select for the diagnostic in the source use a lot of magic numbers, OR an enum-made-just-for it

So if the diagnostic tends to have a lot of /* ThingIWantSelected */ 5 sort of things (or worse, just the 5!), it is likely a good candidate. Ones that are 'calculated' based on some other criteria aren't a great.

One such example is from: #120327 , which is what encouraged this patch.

Once we have ones identified, someone should then go through and convert these diagnostics.

@ayokunle321
Copy link
Contributor

Hey! Kinda new here and would like to work on this.

@erichkeane
Copy link
Collaborator Author

Hey! Kinda new here and would like to work on this.

Great, have fun! Let me know if you have troubles and I'll help how I can! Also, add me to the pull request once you have one.

@ayokunle321
Copy link
Contributor

No p! Could I get assigned?

@shafik
Copy link
Collaborator

shafik commented Jan 16, 2025

No p! Could I get assigned?

done

@amansharma612
Copy link

@ayokunle321 still working on this?

@ayokunle321
Copy link
Contributor

@amansharma612 Nah, would you like to work on it?

@amansharma612
Copy link

@amansharma612 Nah, would you like to work on it?

Yes I can take a look

@shafik shafik assigned amansharma612 and unassigned ayokunle321 Feb 5, 2025
erichkeane pushed a commit that referenced this issue Mar 19, 2025
…elect and adjust its uses (#130868)

Handles #123121

This patch updates `note_constexpr_invalid_cast` diagnostic to use
`enum_select` instead of `select,` improving readability and reducing
reliance on magic numbers in caller sites.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer good first issue https://github.com/llvm/llvm-project/contribute quality-of-implementation
Projects
None yet
Development

No branches or pull requests

6 participants