[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4175263 <https://reviews.llvm.org/D141714#4175263>, @giulianobelinassi wrote:

> Hi, Alan. Thanks for your review again!
>
> With regard to middle, the patch sent in D145269 <https://reviews.llvm.org/D145269> may answer your questions. Basically functions like:
>
>   int* f(void) __attribute__((unused));
>
> would be output as
>
>   int* __attribute__((unused)) f(void);
>
> if SIDE_MIDDLE is specified.

That would be a pretty unique location for those attributes to go and there are some attributes that can't go there. For example: https://godbolt.org/z/fo5cnEGTY

> The case I want to avoid is:
>
>   int f(void) __attribute__((unused))
>     int i;
>   { return 0; }

Did you mean the example to be a K&R C function instead? If so, then this would apply the attribute to the function in Clang.

> Which is not clear where should the attribute be applied to. A left-to-right parser that accepts attributes on the right side of a function declaration may apply this to f, but if it don't then it will be applied to i. This is why GCC rejects this kind of attribute output on the right side of functions.
> Hence, I thought it would be better to always output variable as
>
>   int __attribute__((unused)) var;

That will subtly change program semantics in Clang though, as now the attribute pretty prints to the parameter declaration (I'm still assuming your example was intended to be a K&R C function definition).

> when possible. I also found that __declspec attributes is also accepted this way. But if that changes the meaning of things (I looked into generated assembly and could not find such case) then I think dumping it on the right side may be a better option, and we only have to be careful with functions declarations, those being dumped as.
>
>   __attribute__((unused)) f(void) {}
>
> -------------
>
> As for changing the `__declspec` locations, I thought it was fine as it is also accepted by clang. But looking at MSVC documentation it says that outputing it at the begining is recommended so I think it may be better to always output __declspecs on the left side?

That's where I'm most used to seeing it.

> -------------
>
> Can you explain a bit more about what warning you're seeing and under what condition?
>
> Basically if you change this test in order for it to intentionally fail because of output mismatch, this warning is generated.

Hmmm, I'm still not certain I understand what's going on. What surprises me is that the test code is ` __block StructWithCopyConstructor s(5);` but it pretty prints as `StructWithCopyConstructor __attribute__((blocks("byref"))) s(5);` which doesn't compile at all. That doesn't seem to be new behavior in your patch (and this particular attribute has no documentation or existing test coverage), but I think the fact that the keyword spelling is dropped and the attribute is moved helps point out an issue -- keyword attributes are special and their syntactic position is important, so maybe those should not shift at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714