[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM modulo the suggestion from @rjmccall, thank you for this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added a comment. Minor suggestion for a cleanup, then LGTM. Comment at: clang/lib/Sema/SemaOverload.cpp:9048 +ParamTypes[0] = S.Context.getVolatileType(ParamTypes[0]); + ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]); +

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 updated this revision to Diff 435932. jansvoboda11 added a comment. Implement another round of review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 Files:

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 updated this revision to Diff 435927. jansvoboda11 added a comment. Code review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 Files: clang/include/clang/AST/Type.h

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Type.h:617 +struct QualifiersAndAtomic { + Qualifiers Quals; The set of operations here feel a bit weird. We have `withVolatile` and `withAtomic` but not `withConst` or `withRestrict`.

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added inline comments. Comment at: clang/include/clang/AST/Type.h:277 +return qs; + } Could you go ahead and add this for `const` and `restrict` as well? I think that will give us the full set. Comment at:

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8982 // Extension: Add the binary operators =, +=, -=, *=, /= for vector types. for (QualType Vec1Ty : CandidateTypes[0].vector_types()) jansvoboda11 wrote: >

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 marked an inline comment as done. jansvoboda11 added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8982 // Extension: Add the binary operators =, +=, -=, *=, /= for vector types. for (QualType Vec1Ty : CandidateTypes[0].vector_types())

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 updated this revision to Diff 435675. jansvoboda11 added a comment. Introduce `QualifiersAndAtomic` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 Files: clang/include/clang/AST/Type.h

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8978 + } } } Can we do this in a way that's a little more general if we want to include other qualifiers in this combinatoric explosion in the future? I think

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8982 // Extension: Add the binary operators =, +=, -=, *=, /= for vector types. for (QualType Vec1Ty : CandidateTypes[0].vector_types()) Do we need to handle atomic

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 added a comment. > It doesn't look like the patch handles `volatile _Atomic` correctly, though. That should be fixed in the new revision. > I know we do a lot of candidate prefiltering, and that that can be difficult > because of uesr-defined conversions. How do those things

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 updated this revision to Diff 435448. jansvoboda11 added a comment. Handle volatile _Atomic as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 Files: clang/lib/Sema/SemaOverload.cpp

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added a comment. This seems like the right idea to me, yeah. It doesn't look like the patch handles `volatile _Atomic` correctly, though. I know we do a lot of candidate prefiltering, and that that can be difficult because of uesr-defined conversions. How do those things interact

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 added a comment. I tried the approach suggested by @ahatanak. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 ___ cfe-commits mailing list

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 updated this revision to Diff 435108. jansvoboda11 added a comment. Herald added a project: clang. Add operators for atomic types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 Files:

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jansvoboda11 commandeered this revision. jansvoboda11 added a reviewer: jkorous. jansvoboda11 added a comment. Jan has asked me to take over. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125349/new/ https://reviews.llvm.org/D125349 ___

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jkorous added a comment. In D125349#3509073 , @aaron.ballman wrote: > It's interesting to note that `an_atomic_uint = an_atomic_uint + > an_enum_value` works correctly: https://godbolt.org/z/cvP9e6nh7. I was trying > to figure out whether the atomic

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

jkorous added a comment. In D125349#3509546 , @ahatanak wrote: > Is it not possible to handle this similarly to `volatile unsigned`? If I > replace `_Atomic unsigned` with `volatile unsigned`, I see > `LookupOverloadedBinOp` succeed without having to

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

ahatanak added a comment. Is it not possible to handle this similarly to `volatile unsigned`? If I replace `_Atomic unsigned` with `volatile unsigned`, I see `LookupOverloadedBinOp` succeed without having to strip volatile because `addAssignmentArithmeticOverloads` adds candidates with