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

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

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