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

[clang][Sema] Emit more specific diagnostic for auto in lambda before C++14 (#46059) #68540

Merged

Conversation

weltschildkroete
Copy link
Contributor

@weltschildkroete weltschildkroete commented Oct 8, 2023

Namely, we specify that auto in a lambda parameter is a C++14 extension in the error message, which now reads:

'auto' not allowed in lambda parameter before C++14

This does not change the behavior for decltype(auto) and __auto_type though.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 8, 2023
@cor3ntin
Copy link
Contributor

cor3ntin commented Oct 9, 2023

Thanks for this patch!

Our policy is to not mix functional changes and formatting changes.
Could you change the PR to have only the changes you made, wit no formatting change?
You can format your patch with git clang-format which only formats the lines that you changed.

@weltschildkroete weltschildkroete force-pushed the diagnostic-auto-in-lambda-param-c++11 branch from 3690ee2 to 7e62fe6 Compare October 9, 2023 23:18
@weltschildkroete
Copy link
Contributor Author

Thanks for this patch!

Thank you for the feedback and for your patience. :-)

It should be fixed now (hopefully!)

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

A small nit, otherwise LGTM

I'm not sure it's strictly necessary here, but you could add a release note in clang/docs/ReleaseNotes.rst - under the diagnostics improvement section

@@ -2393,7 +2393,7 @@ def err_auto_not_allowed : Error<
"|in type allocated by 'new'|in K&R-style function parameter"
"|in template parameter|in friend declaration|in function prototype that is "
"not a function declaration|in requires expression parameter"
"|in array declaration}1">;
"|in array declaration|in lambda parameter until C++14}1">;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"|in array declaration|in lambda parameter until C++14}1">;
"|in array declaration|in lambda parameter before C++14}1">;

To be consistent with other diagnostics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

… C++14 (llvm#46059)

Namely, we specify that `auto` in a lambda parameter is a C++14
extension in the error message, which now reads:

`'auto' not allowed in lambda parameter until C++14`

This does not change the behavior for `decltype(auto)` and `__auto_type`
though.

The relevant change to `SemaType.cpp` is the addition of a branch that
sets `Error = 24`, whilst the bulk of the change comes from formatting.
@weltschildkroete weltschildkroete force-pushed the diagnostic-auto-in-lambda-param-c++11 branch from 7e62fe6 to 99bc3b6 Compare October 10, 2023 21:16
@weltschildkroete
Copy link
Contributor Author

LMK if the release note sounds good. If so, could you land this patch for me? Please use "Leonardo Duarte weltschildkroete@gmail.com" to commit the change. Thanks!

Auto->getKeyword() != AutoTypeKeyword::Auto) {
if (!SemaRef.getLangOpts().CPlusPlus14 && Auto &&
Auto->getKeyword() == AutoTypeKeyword::Auto) {
Error = 24;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of the other cases have comments explaining what diagnostic the code pertains to. See 20 or 7 below for example.

This is not really great but at least we have a guide to which diagnostic we intend to emit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I added some similar comments as well.

@github-actions
Copy link

github-actions bot commented Oct 11, 2023

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

@cor3ntin
Copy link
Contributor

@shafik I'll let you have the final say :)

@cor3ntin
Copy link
Contributor

@shafik ping

@shafik
Copy link
Collaborator

shafik commented Apr 4, 2024

@shafik ping

Apologies, this got lost on my end.

@cor3ntin cor3ntin merged commit 476f7f6 into llvm:main May 16, 2024
4 of 5 checks passed
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.

None yet

4 participants