[PATCH] D38816: Convert clang::LangAS to a strongly typed enum

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.


================
Comment at: lib/AST/TypePrinter.cpp:1323
     OS << "address_space(";
-    OS << T->getEquivalentType().getAddressSpace();
+    OS << T->getEquivalentType()
+              .getQualifiers()
----------------
yaxunl wrote:
> arichardson wrote:
> > arichardson wrote:
> > > Anastasia wrote:
> > > > arichardson wrote:
> > > > > Anastasia wrote:
> > > > > > arichardson wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > Why do we need this change?
> > > > > > > `__attribute__((address_space(n)))` is a target address space and not a language address space like `LangAS::opencl_generic`. Isn't `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this use case?
> > > > > > Yes, I think there are some adjustment we do in this method to get the original source value to be printed corerctly. Does this mean we have no tests that caught this issue?
> > > > > Seems like it, all tests pass both with and without this patch.
> > > > Strange considering that we have this attribute printed in some error messages of some Sema tests. If I compile this code without your patch:
> > > >  
> > > > ```
> > > > typedef int __attribute__((address_space(1))) int_1;
> > > > typedef int __attribute__((address_space(2))) int_2;
> > > > 
> > > > void f0(int_1 &); 
> > > > void f0(const int_1 &);
> > > > 
> > > > void test_f0() {
> > > >   int i;
> > > >   static int_2 i2;
> > > >   f0(i);
> > > >   f0(i2);
> > > > }
> > > > ```
> > > > 
> > > > I get the address spaces printed correctly inside the type:
> > > >   note: candidate function not viable: 1st argument ('int_2' (aka '__attribute__((address_space(2))) int')) is in address space 2, but parameter must be in address space 1
> > > > 
> > > > Perhaps @yaxunl could comment further on whether this change is needed.
> > > My guess is that it doesn't go through that switch statement but rather through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to see if there are any tests that go down this path.
> > I just ran the clang tests with an llvm_unreachable() here and none of them failed. So it seems like we don't have anything testing this code path.
> Sorry for the delay. This part of code is for printing the addr space of AttributedType. Since it seems not used by any language yet, there is no test for it. It is possible a non-target-specific address space being printed here if a language chooses to use AttributedType to represent address space. Therefore a proper fix would be isolate the code for printing address space from Qualifiers::print and re-use it here so that addr space is printed in consistent way no matter it is represented as qualifier or as AttributedType.
Thanks, that makes sense. To avoid breaking anything here I think it should be part of a separate patch though.


https://reviews.llvm.org/D38816