aaron.ballman added inline comments.

================
Comment at: clang/test/Sema/address_space_attribute.cpp:9
+  // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+  __attribute__((address_space(1))) int *x;
+
----------------
leonardchan wrote:
> aaron.ballman wrote:
> > Can you also add a test using the `[[clang::address_space(1)]]` spelling 
> > and ensure that it is printed properly?
> I do not think `address_space` has double bracket spelling. Is there a 
> specific attribute under Attr.td or other td file that specifies if an 
> attribute is supported with c++ spelling?
All attributes with the `Clang` spelling are given a GNU-style 
(`__attribute__((foo))`) spelling and a double-square bracket 
(`[[clang::foo]]`) spelling in the clang vendor namespace (for both C++ and C). 
Attributes with the `GCC` spelling are similar in that they provide a GNU-style 
and double-square bracket spelling (in the `gnu` vendor namespace), though the 
`[[]]` spelling is currently only for C++.


================
Comment at: clang/tools/libclang/CXType.cpp:132
+      if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+          ATT->getAttrKind() != attr::AddressSpace) {
         return MakeCXType(ATT->getModifiedType(), TU);
----------------
leonardchan wrote:
> aaron.ballman wrote:
> > This change seems surprising -- if the parsing options say the caller does 
> > not want attributed types, why are we returning one anyway for address 
> > space?
> This has to do with ensuring `clang_getAddressSpace` still returns the proper 
> address_space. It does this by essentially checking the qualifiers of the 
> type, which we now attach to the `AttributedType` whereas before it was 
> attached to the modified type.
> 
> This extra condition is necessary for ensuring that calling 
> `clang_getAddressSpace` points to the qualified AttributedType instead of the 
> unqualified modified type.
My fear is that this will be breaking assumptions in third-party code. If 
someone disables `CXTranslationUnit_IncludeAttributedTypes`, they are unlikely 
to expect to receive an `AttributedType` and may react poorly to it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55447



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to