rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land.
LGTM, but you can probably undo one of my requests. :) ================ Comment at: lib/AST/Type.cpp:2950 + FunctionTypeBits.HasExtQuals = 0; + } } ---------------- mikael wrote: > mikael wrote: > > rjmccall wrote: > > > 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. > > I'll change it, but I do have a question related to this: > > > > Can we make any assumptions with the "fast qualifiers"? I'd like it if we > > can assume that they fit in 3 bits. Otherwise I think I also need an > > assertion here. > I solved it like this in the end: > > * I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to > make it dependent on possible changes to the fast flags. > * Then I put a static_assertion to guard the hastConst(), hasVolatile() and > hasRestrict() methods. Great, looks good. ================ Comment at: lib/CodeGen/CGDebugInfo.cpp:2593 + getOrCreateInstanceMethodType( + CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()), + FPT, U), ---------------- mikael wrote: > rjmccall wrote: > > Why is this `getMostRecentCXXRecordDecl` instead of `getClass`? > Your initial comment suggested to send in a record rather than a type. But I > see now that it may make more sense to use the type directly instead. Will > update it. Oh, I actually didn't even think about the fact that the `getClass())` returns a `Type*` instead of a `CXXRecordDecl`. Using the record decl is probably better, sorry. 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
