[PATCH] D43248: [Attr] Fix parameter indexing for attributes

[Joel E. Denny via Phabricator via cfe-commits]

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