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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Attr.h:206
+
+  void cmpable(const ParamIdx &I) const {
+    assert(isValid() && I.isValid() &&
----------------
jdenny wrote:
> 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?
Derp, you're correct, it's already implicitly inline. Ignore that part of the suggestion.


================
Comment at: include/clang/AST/Attr.h:236
+  /// parameter.
+  ParamIdx(unsigned Idx, bool HasThis)
+      : Idx(Idx), HasThis(HasThis), IsValid(true) {}
----------------
jdenny wrote:
> 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.
That'd explain why I hadn't seen it.


================
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; }
----------------
jdenny wrote:
> 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.
I don't think it's actively harmful to do so, but I also don't think it's really needed either. Your call.


================
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.
----------------
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?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4549-4551
+    if (ArgumentIdx.getASTIndex() >= getFunctionOrMethodNumParams(D) ||
+        !getFunctionOrMethodParamType(D, ArgumentIdx.getASTIndex())
+             ->isPointerType())
----------------
jdenny wrote:
> aaron.ballman wrote:
> > Is this formatting produced by clang-format?
> Yes.
How funky. :-)


https://reviews.llvm.org/D43248