jdenny added a comment. In https://reviews.llvm.org/D43248#1024183, @aaron.ballman wrote:
> In https://reviews.llvm.org/D43248#1023720, @jdenny wrote: > > > Hi Aaron. It occurs to me now that this patch has grown rather large and, > > in some places, a little subtle. Would it help the review if I were to > > break it up into a patch series that introduces ParamIdx to each attribute, > > one at a time? I'm not trying to rush you, but I hate for the review to be > > painful for you if it doesn't have to be. > > > No need to do that -- this review just takes a bit more time for me to > complete, but it's reasonably well-factored. Thank you, though! Sure! Thanks for the review. ================ Comment at: include/clang/AST/Attr.h:206 + + void cmpable(const ParamIdx &I) const { + assert(isValid() && I.isValid() && ---------------- aaron.ballman wrote: > The name here can be improved. How about `checkInvariants()`? Might as well > make this inline while you're at it. Sure, I can change the name. It's inside the class, so specifying inline is against the LLVM coding standards, right? ================ Comment at: include/clang/AST/Attr.h:236 + /// parameter. + ParamIdx(unsigned Idx, bool HasThis) + : Idx(Idx), HasThis(HasThis), IsValid(true) {} ---------------- aaron.ballman wrote: > Is this constructor used anywhere? I didn't see it being used, but I could > have missed something. If it's not used, go ahead and remove it. It's used by the deserialization code generated by ClangAttrEmitter.cpp. ================ Comment at: include/clang/AST/Attr.h:281-284 + bool operator<(const ParamIdx &I) const { cmpable(I); return Idx < I.Idx; } + bool operator>(const ParamIdx &I) const { cmpable(I); return Idx > I.Idx; } + bool operator<=(const ParamIdx &I) const { cmpable(I); return Idx <= I.Idx; } + bool operator>=(const ParamIdx &I) const { cmpable(I); return Idx >= I.Idx; } ---------------- aaron.ballman wrote: > Are all of these operations required for the class? operator== and operator< are needed for sorting and finding. It seems strange to me not to finish the set. ================ Comment at: include/clang/Basic/Attr.td:172-174 + // Whether the C++ implicit this parameter is allowed. Users that construct + // attributes from the source code use this information when validating + // parameter indices. ---------------- aaron.ballman wrote: > I still find the `AllowThis` parts to be hard to follow, so I want to be sure > I understand your design properly. Everything that uses this new argument > type sets `AllowsThis` to 0. As written, it sounds like setting that to 0 > means that the parameter index cannot be used on a C++ instance method, and I > know that's not the correct interpretation. Under what circumstances would > this be set to 1 instead? > > Looking at the existing code, the only circumstances under which > `checkFunctionOrMethodParameterIndex()` was being passed `true` for "allow > this" was for XRayLogArgs, which doesn't even use `ParamIdx`. Perhaps this > member isn't necessary any longer? > I still find the AllowThis parts to be hard to follow, so I want to be sure I > understand your design properly. Everything that uses this new argument type > sets AllowsThis to 0. As written, it sounds like setting that to 0 means that > the parameter index cannot be used on a C++ instance method, and I know > that's not the correct interpretation. Right. AllowsThis=0 means it is an error for the attribute in the source code to specify the C++ implicit this parameter (index 1). > Under what circumstances would this be set to 1 instead? > > Looking at the existing code, the only circumstances under which > checkFunctionOrMethodParameterIndex() was being passed true for "allow this" > was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this member > isn't necessary any longer? Right. I also noticed this issue, but I probably should have mentioned that in a comment in the source instead of just rewording the last paragraph of the patch summary. Sorry. I thought about removing AllowsThis, but I hesitated because I had already implemented it by the time I noticed this issue and because I assumed there must be some reason why attributes for C++ have index 1 mean the this parameter, so there might be some attribute that could eventually take advantage of AllowsThis=1. Moreover, it makes the related argument to checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer. I don't feel strongly either way, so I'm happy to remove it or keep it. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1650-1651 OwnershipAttr(AL.getLoc(), S.Context, nullptr, nullptr, 0, - AL.getAttributeSpellingListIndex()).getOwnKind(); + AL.getAttributeSpellingListIndex()) + .getOwnKind(); ---------------- aaron.ballman wrote: > This change looks to be unrelated to the patch? Sorry, I think clang-format suggested that at some point in history, and I overlooked it when reviewing the latest version. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:4549-4551 + if (ArgumentIdx.getASTIndex() >= getFunctionOrMethodNumParams(D) || + !getFunctionOrMethodParamType(D, ArgumentIdx.getASTIndex()) + ->isPointerType()) ---------------- aaron.ballman wrote: > Is this formatting produced by clang-format? Yes. https://reviews.llvm.org/D43248 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
