[PATCH] D147989: [clang] Fix Attribute Placement

This revision was automatically updated to reflect the committed changes. Closed by commit rGb893368fd4fd: Fix the diagnostic about attribute placement for scoped enumerations (authored by ipriyanshi1708, committed by aaron.ballman). Changed prior to commit:

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 added a comment. In D147989#4299722 , @aaron.ballman wrote: > LGTM! Do you need someone to land this on your behalf? If so, what name and > email address would you like used for patch attribution? Yes, I need someone who can commit it

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution? Repository: rG LLVM Github Monorepo

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 added a comment. In D147989#4295928 , @aaron.ballman wrote: > The issue is that `GetDiagnosticTypeSpecifierID()` is called for more > diagnostics than just `warn_declspec_attribute_ignored`. You need to also > update the

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 517207. ipriyanshi1708 added a comment. Fixed the code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files: clang/docs/ReleaseNotes.rst

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman added a comment. The issue is that `GetDiagnosticTypeSpecifierID()` is called for more diagnostics than just `warn_declspec_attribute_ignored`. You need to also update the `err_constexpr_tag` and `err_standalone_class_nested_name_specifier` to add `enum class` and `enum struct`

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 marked an inline comment as done. ipriyanshi1708 added a comment. In D147989#4293278 , @aaron.ballman wrote: > Oops, I spoke too soon -- it looks like the precommit CI failure is related > to this patch. This is what I get when I tested

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Oops, I spoke too soon -- it looks like the precommit CI failure is related to this patch. This is what I get when I tested locally: Assertion failed: NextVal !=

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution? (I can fix the tiny style nit myself when landing,

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 516037. ipriyanshi1708 marked 4 inline comments as done. ipriyanshi1708 added a comment. Removed unwanted changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 516036. ipriyanshi1708 added a comment. Added case for enum struct Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files: clang/docs/ReleaseNotes.rst

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Thank you for working on this! I spotted a little more that needs to be done, but this is pretty close to ready. I *think* the precommit CI failures are unrelated to

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 marked 3 inline comments as done. ipriyanshi1708 added a comment. In D147989#4283666 , @samtebbs wrote: > This looks good to me now, nice work. Let's wait a few days for others' input > to be safe. okay! Thank You Sir.

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs accepted this revision. samtebbs added a comment. This revision is now accepted and ready to land. This looks good to me now, nice work. Let's wait a few days for others' input to be safe. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 515196. ipriyanshi1708 marked an inline comment as done. ipriyanshi1708 added a comment. Updated the logic in a better way Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 514931. ipriyanshi1708 marked an inline comment as done. ipriyanshi1708 added a comment. Updated the logic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5041 case DeclSpec::TST_enum: return 4; default: ipriyanshi1708 wrote: > jrtc27 wrote: > > Why not just always pass the full DeclSpec and handle the class case here, > > maybe

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5051 +return 5; + else +return 4; Instead of returning 4 here, I think it's best to just delegate to the other `GetDiagnosticTypeSpecifierID` function. That way, if the

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 514911. ipriyanshi1708 added a comment. Removed spurious whitespace changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files:

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 514909. ipriyanshi1708 added a comment. Updated the test file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files: clang/docs/ReleaseNotes.rst

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 added a comment. In D147989#4257721 , @aaron.ballman wrote: > Thank you for working on this! > > The changes are missing test coverage; please be sure to add that, along with > a release note about the fix. I think there's likely more

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 513549. ipriyanshi1708 marked 5 inline comments as done. ipriyanshi1708 added a comment. Improved the logic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989

[PATCH] D147989: [clang] Fix Attribute Placement

jrtc27 added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5041 case DeclSpec::TST_enum: return 4; default: Why not just always pass the full DeclSpec and handle the class case here, maybe with a bool to allow other users to not need to

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman added a comment. Precommit CI has several failures introduced by this patch -- some test cases need to be fixed up (in addition to the new test coverage). Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3424 "attribute %0 is ignored, place it

[PATCH] D147989: [clang] Fix Attribute Placement

tbaeder added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3424 "attribute %0 is ignored, place it after " - "\"%select{class|struct|interface|union|enum}1\" to apply attribute to " + "\"%select{class|struct|interface|union|enum|enum

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512445. ipriyanshi1708 added a comment. Applied C++ Overloading Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files: clang/docs/ReleaseNotes.rst

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 added a comment. In D147989#4257947 , @samtebbs wrote: > Thanks for the input Aaron and Martin. This is looking good. With the > suggested changes and some formatting > I

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512437. ipriyanshi1708 added a comment. Added the Release note for the fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files:

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs added a comment. Thanks for the input Aaron and Martin. This is looking good. With the suggested changes (don't forget the test, you could try adding one to clang/test/Sema/attr-declspec-ignored.c and clang/test/SemaCXX/attr-declspec-ignored.cpp) and some formatting

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 added a comment. In D147989#4257721 , @aaron.ballman wrote: > Thank you for working on this! > > The changes are missing test coverage; please be sure to add that, along with > a release note about the fix. I think there's likely more

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512421. ipriyanshi1708 added a comment. Removed the spurious whitespace changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files:

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512419. ipriyanshi1708 added a comment. Implemented the required changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989 Files:

[PATCH] D147989: [clang] Fix Attribute Placement

mboehme added a comment. In D147989#4257539 , @samtebbs wrote: > Looping in some people who have worked on and reviewed patches in this area > for some experienced eyes. Nothing else to add beyond what @aaron.ballman has noted. Thanks for working on

[PATCH] D147989: [clang] Fix Attribute Placement

aaron.ballman added a comment. Thank you for working on this! The changes are missing test coverage; please be sure to add that, along with a release note about the fix. I think there's likely more work to be done here as well, considering this behavior: https://godbolt.org/z/sdK3Gef75 (notice

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs added reviewers: mboehme, aaron.ballman. samtebbs added a comment. Looping in some people who have worked on and reviewed patches in this area for some experienced eyes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/

[PATCH] D147989: [clang] Fix Attribute Placement

samtebbs added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:58 using namespace sema; - +bool isEnumClass = false; Sema::DeclGroupPtrTy Sema::ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType) { This being global could lead to issues where one

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512349. ipriyanshi1708 edited the summary of this revision. ipriyanshi1708 added a comment. Updated the summary Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147989/new/ https://reviews.llvm.org/D147989

[PATCH] D147989: [clang] Fix Attribute Placement

ipriyanshi1708 updated this revision to Diff 512344. ipriyanshi1708 retitled this revision from "Fix Attribute Placememt" to "[clang] Fix Attribute Placement". ipriyanshi1708 added a comment. Updated the title Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION