jdenny marked 3 inline comments as done. jdenny added inline comments.
================ 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: > jdenny wrote: > > 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. > > Right. AllowsThis=0 means it is an error for the attribute in the source > > code to specify the C++ implicit this parameter (index 1). > > Then if we keep this functionality, I think a better identifier would be > something like `CanIndexImplicitThis` and the comments could be updated to > more clearly state what is allowed/disallowed. Then the other uses of "allow > this" can be updated to use similar terminology for clarity. > > > 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. > > My gut feeling is that this functionality isn't going to be needed all that > frequently. If we get a second case where we need it, then I'd say it might > make more sense to add it. > > Attributes that use positional arguments should hopefully be the exception, > not the rule, because those indexes are horribly fragile. > > What do you think? > My gut feeling is that this functionality isn't going to be needed all that > frequently. If we get a second case where we need it, then I'd say it might > make more sense to add it. > > Attributes that use positional arguments should hopefully be the exception, > not the rule, because those indexes are horribly fragile. > > What do you think? I'm guessing you have more experience with attributes in general, so let's go with your gut. :-) ================ Comment at: lib/Sema/SemaDeclAttr.cpp:4549-4551 + if (ArgumentIdx.getASTIndex() >= getFunctionOrMethodNumParams(D) || + !getFunctionOrMethodParamType(D, ArgumentIdx.getASTIndex()) + ->isPointerType()) ---------------- aaron.ballman wrote: > jdenny wrote: > > aaron.ballman wrote: > > > Is this formatting produced by clang-format? > > Yes. > How funky. :-) Agreed. https://reviews.llvm.org/D43248 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
