rjmccall added inline comments.
================
Comment at: lib/AST/ItaniumMangle.cpp:1507
+ Qualifiers MethodQuals = Qualifiers::fromCVRUMask(
+ Method->getTypeQualifiers().getCVRUQualifiers());
// We do not consider restrict a distinguishing attribute for overloading
----------------
mikael wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > You can overload based on the address space, right? I think it needs
> > > > to be mangled.
> > > Does this refer to our earlier discussion
> > > https://reviews.llvm.org/D54862#inline-484509
> > >
> > > We don't have a way to qualify methods with an address space yet? I was
> > > going to send an RFC to `cfe-dev` for this but if you think it would be
> > > ok to go ahead with an implementation, I am happy with it. Either way
> > > would it be better to do this in a separate patch?
> > I'm fine with delaying implementation on these two issues until a later
> > patch since, as you say, they can't be tested well until we support
> > arbitrary address-space qualifiers. Please at least leave FIXMEs for them.
> I actually did update this, since we can test the mangling of __generic in
> the test.
Great, thanks. Looks good.
================
Comment at: lib/AST/Type.cpp:2950
+ FunctionTypeBits.HasExtQuals = 0;
+ }
}
----------------
The indentation here is messed up.
You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few
places. That's currently correct, in the sense that the fast qualifiers are
currently defined to be the CVR qualifiers, but the point of the distinction is
that we might want to change that (and we have in fact explored that in the
past). I think you should make `FunctionTypeBits` only claim to store the fast
qualifiers, which mostly just means updating a few field / accessor names and
changing things like the `getCVRQualifiers()` call right above this to be
`getFastQualifiers()`.
If there are places where it's convenient to assume that the CVR qualifiers are
exactly the fast qualifiers, it's okay to static_assert that, but you should
make sure to static_assert it in each place you assume it.
================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+ getOrCreateInstanceMethodType(
+ CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+ FPT, U),
----------------
Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
================
Comment at: lib/CodeGen/CGDeclCXX.cpp:32
+ (D.hasLocalStorage() &&
CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+ "VarDecl must have global or local (in the case of OpenCL) storage!");
assert(!D.getType()->isReferenceType() &&
----------------
What causes OpenCL C++ declarations to go down this path?
================
Comment at: lib/Index/USRGeneration.cpp:274
+ if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
Out << (char)('0' + quals);
switch (MD->getRefQualifier()) {
----------------
akyrtzi wrote:
> rjmccall wrote:
> > Paging @akyrtzi here. The address-space qualifier should be part of the
> > USR but I don't think if there's a defined schema for that.
> If I understand correctly from other comments you can't add special mangling
> and USR generation yet and intend to add FIXME for doing mangling support
> later ? If yes, could you also add FIXME here and re-visit ?
While you're here, can you described the right way for us to extend USR
generation here to potentially add an address space?
================
Comment at: lib/Parse/ParseDecl.cpp:6166
+
+ Qualifiers Q = Qualifiers::fromOpaqueValue(DS.getTypeQualifiers());
+ if (D.getDeclSpec().isConstexprSpecified() && !getLangOpts().CPlusPlus14)
----------------
I think `fromCVRUQualifiers` is probably the right function, and if that
doesn't exist you should add it.
================
Comment at: lib/Sema/SemaExprCXX.cpp:1112
+ QualType T = S.Context.getRecordType(Record).withCVRQualifiers(Quals);
+ T = S.getASTContext().getAddrSpaceQualType(T,
CXXThisTypeQuals.getAddressSpace());
+
----------------
I don't think there's a good reason for this to be dropping non-CVR qualifiers.
If there is, it really needs a comment.
================
Comment at: lib/Sema/TreeTransform.h:5276
+ Sema::CXXThisScopeRAII ThisScope(
+ SemaRef, ThisContext, Qualifiers::fromOpaqueValue(ThisTypeQuals));
----------------
I think you need to turn `ThisTypeQuals` into a `Qualifiers` here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54862/new/
https://reviews.llvm.org/D54862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
