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