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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.


================
Comment at: test/Sema/attr-micromips.c:9
 
-__attribute__((micromips,mips16)) void foo5();  // expected-error {{'micromips' and 'mips16' attributes are not compatible}} \
+__attribute__((micromips,mips16)) void foo5();  // expected-error {{'mips16' and 'micromips' attributes are not compatible}} \
                                                 // expected-note {{conflicting attribute is here}}
----------------
echristo wrote:
> This seems to reverse? What's going on here? There are other occurrences too.
This is caused by the same phenomenon as the 'previous' marker changing to the attribute that is textually after the main marker. The order within the same message seems less important to me.

For following happens when attributes are merged:
1. If the attributes are of the same declaration, compatibility is checked with the existing attributes. With the current order: textually later attributes are already in the list of accepted attributes and textually earlier attributes are added to it.
2. If the attributes are in two different declarations, the new (textually later) `clang::Decl` with its attributes is taken and the attributes of the old (textually earlier) clang::Decl are added to it while checking for compatibility.

That is, in both cases the textually earlier attribute is added to the existing list of textually later attributes. The diagnostics are printed with the exiting attribute on the right and the attribute-to-be added on the left (at least in this case. `test/Sema/attr-swiftcall.c:12` is a counterexample where this patch changes it to the textual order).

Conflicts are resolved by throwing away the conflicting attribute in the list (i.e. textually later) to make room for the new (textually earlier) attribute.

This does not seem to have evolved intentionally when picking in which order to print to two conflicting attributes. I would prefer if the the AST represents the source as accurately as possible, including ordering of attributes (e.g. `test/Sema/attr-print.c`), which in some cases carry semantic meaning: `EnableIfAttr` and `LoopHintAttr`. Today, when dumping the AST, these are just incorrect.

I spent some time trying to reverse the order in which the attributes appear in the diagnostic messages (including `previous occurance here`), but this would also require changing case 2., which is more difficult.



Repository:
  rC Clang

https://reviews.llvm.org/D48100