thieta marked an inline comment as done.
thieta added a comment.
Thanks for your comments - feel free to comment on the release note, I was
struggling with describing the fix well for a short release note paragraph.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:61-63
+ for (ParsedAttr &AL : DS.getAttributes())
+ if (AL.isDeclspecAttribute())
+ ToBeMoved.push_back(&AL);
----------------
aaron.ballman wrote:
> How about using an algorithm for this (would need to be reformatted for 80
> col)?
I spent a while on trying to figure this out. I am not that great with the
algorithms in the standard library since we don't really use that at work. But
from my testing I couldn't get it to work:
- DS.getAttributes() returns a `ParsedAttr&`, but ToBeMoved needs a pointer to
ParsedAttr. So your example code above fails because it can't convert
ParsedAttr& to ParsedAttr* in the back_inserter()
- I tried to change ToBeMoved to be a list of references instead, but
ParsedAttr disallows copying (I think, the error message was a bit confusing).
- I could do transform() to a list of pointers and then copy_if() but that
seemed to really make it harder to understand.
- A transform_if() would solve the problem or I guess replace back_inserter()
with some out iterator that could transform. But I couldn't find anything used
like this browsing the LLVM source code.
So yeah - I might totally be missing something obvious here, feel free to point
it out.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:65-66
+
+ for (ParsedAttr *AL : ToBeMoved)
+ Attrs->takeOneFrom(DS.getAttributes(), AL);
+
----------------
aaron.ballman wrote:
> Similar here, but this one might be less of a win in terms of readability.
yeah I can do this - I'll wait to see what we do with the block above first.
================
Comment at: clang/test/SemaCXX/using-declspec.cpp:4
+
+struct Test { int a; };
+using AlignedTest = __declspec(align(16)) const Test;
----------------
aaron.ballman wrote:
> Because we're touching `ParseTypeName()` and that is called in a lot of
> contexts, I think we should have extra testing in non-alias declaration cases
> and verify that MSVC behaves the same way. e.g., (search for
> `ParseTypeName()`, see where we parse type names, devise some test cases)
> ```
> // I don't think the align attribute has a declaration to move onto in this
> case...
> auto func() -> __declspec(align(16)) int;
> // So I *think* the alignment of the return type isn't changed?
> static_assert(alignof(decltype(func())) == alignof(int));
>
> // Same here, no declaration to shift to
> int i = (__declspec(align(16))int)12;
>
> // But there is a declaration here!
> typedef __declspec(align(16)) int Foo;
> static_assert(alignof(Foo) == 16);
> ```
> (There are possibly other interesting tests to consider.)
>
> I suspect we should be issuing some "attribute ignored" diagnostics for the
> cases where the attribute has no effect (even if MSVC does not warn).
Thanks for the expanded tests here! I tried all these tests on MSVC and they
behave exactly as you described them above and they found bug in my
implementation - Attrs in ParseTypeName() could be a nullptr.
We already emit warnings where the attribute is not used as you can see in the
test case below.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143632/new/
https://reviews.llvm.org/D143632
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
