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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
