[PATCH] D143632: [clang] Handle __declspec() attributes in using

[Tobias Hieta via Phabricator via cfe-commits]

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