[PATCH] D48100: Append new attributes to the end of an AttributeList.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked an inline comment as done.
Meinersbur added inline comments.


================
Comment at: test/Sema/attr-ownership.c:22
 void f15(int, int)
-  __attribute__((ownership_returns(foo, 1)))  // expected-note {{declared with index 1 here}}
-  __attribute__((ownership_returns(foo, 2))); // expected-error {{'ownership_returns' attribute index does not match; here it is 2}}
+  __attribute__((ownership_returns(foo, 1)))  // expected-error {{'ownership_returns' attribute index does not match; here it is 1}}
+  __attribute__((ownership_returns(foo, 2))); // expected-note {{declared with index 2 here}}
----------------
erichkeane wrote:
> This seems wrong to me, the 2nd one should be the error condition, right?
This is the result of how attributes are combined. There are the two possibilities
```
void f15(int, int) __attribute__((ownership_returns(foo, 1)));
void f15(int, int) __attribute__((ownership_returns(foo, 2)));
```
and
```
void f15(int, int) __attribute__((ownership_returns(foo, 1)))
                   __attribute__((ownership_returns(foo, 2)));
```

The error diagnosis seem to have been written with the first case in mind and emits in the order there. In the second case attributes merged in the other order (which is the naively correct order, see https://reviews.llvm.org/D48100#1142865), resulting diagnosis to be emitted in the reverse-textual order. There is no consistency in which SourceLocation is the first and which one is the second, and many diagnoses are already not printed in textual order.

I cannot change this without massively reworking how attributes are processed in clang.


================
Comment at: test/Sema/attr-print.c:25
 
 // TODO: the Type Printer has no way to specify the order to print attributes
 // in, and so it currently always prints them in reverse order. Fix this.
----------------
erichkeane wrote:
> This TODO doesn't apply, right?  Or is at least wrong...
Correct, I missed this todo.


================
Comment at: test/Sema/attr-visibility.c:18
 
-void test6() __attribute__((visibility("hidden"), // expected-note {{previous attribute is here}}
-                            visibility("default"))); // expected-error {{visibility does not match previous declaration}}
+void test6() __attribute__((visibility("default"), // expected-error {{visibility does not match previous declaration}}
+                            visibility("hidden"))); // expected-note {{previous attribute is here}}
----------------
erichkeane wrote:
> This order issue is likely to appear a couple of times I suspect.
See my previous reply and https://reviews.llvm.org/D48100#1142865


================
Comment at: test/SemaOpenCL/address-spaces.cl:64
   typedef __private int private_int_t;
-  __local __private int var1;   // expected-error {{multiple address spaces specified for type}}
-  __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
+  __private __local int var1;   // expected-error {{multiple address spaces specified for type}}
+  __private __local int *var2;  // expected-error {{multiple address spaces specified for type}}
----------------
erichkeane wrote:
> These changes have me concerned... The error message isn't specific, but we have to change the order anyway?
An additional error is emitted with the reverse order (`non-kernel function variable cannot be declared in local address space`; a result of which attribute 'wins' in error resolution which is again related to which attribute is already in the AttributeList). I'd say it is the responsibility diagnosis code to emit the same set of messages independent of attribute order which I do not try to fix here.


Repository:
  rC Clang

https://reviews.llvm.org/D48100