[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() &&
----------------
The name here can be improved. How about `checkInvariants()`? Might as well make this inline while you're at it.


================
Comment at: include/clang/AST/Attr.h:236
+  /// parameter.
+  ParamIdx(unsigned Idx, bool HasThis)
+      : Idx(Idx), HasThis(HasThis), IsValid(true) {}
----------------
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.


================
Comment at: include/clang/AST/Attr.h:267
+    assert(isValid() && "ParamIdx must be valid");
+    return Idx - 1 - HasThis;
+  }
----------------
Please assert that `Idx` won't wrap before doing the return.


================
Comment at: include/clang/AST/Attr.h:276
+    assert(isValid() && "ParamIdx must be valid");
+    return Idx - 1;
+  }
----------------
Likewise here.


================
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; }
----------------
Are all of these operations required for the class?


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


================
Comment at: lib/Sema/SemaChecking.cpp:2622
 
-      for (unsigned Val : NonNull->args()) {
-        if (Val >= Args.size())
+      for (ParamIdx Idx : NonNull->args()) {
+        if (Idx.getASTIndex() >= Args.size())
----------------
`const ParamIdx &`


================
Comment at: lib/Sema/SemaChecking.cpp:10083
 
-          for (unsigned ArgNo : NonNull->args()) {
-            if (ArgNo == ParamNo) {
+          for (ParamIdx ArgNo : NonNull->args()) {
+            if (ArgNo.getASTIndex() == ParamNo) {
----------------
`const ParamIdx &`


================
Comment at: lib/Sema/SemaChecking.cpp:12244
   }
-  const Expr *TypeTagExpr = ExprArgs[Attr->getTypeTagIdx()];
+  const Expr *TypeTagExpr = ExprArgs[Attr->typeTagIdx().getASTIndex()];
   bool FoundWrongKind;
----------------
Hoist the AST index so you don't have to call for it twice. (Same applies elsewhere.)


================
Comment at: lib/Sema/SemaDeclAttr.cpp:785-786
-  const ParmVarDecl *Param = FD->getParamDecl(Idx);
-  if (AllowDependentType && Param->getType()->isDependentType())
-    return true;
   if (!Param->getType()->isIntegerType() && !Param->getType()->isCharType()) {
----------------
Good catch about this not being needed any longer!


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1650-1651
       OwnershipAttr(AL.getLoc(), S.Context, nullptr, nullptr, 0,
-                    AL.getAttributeSpellingListIndex()).getOwnKind();
+                    AL.getAttributeSpellingListIndex())
+          .getOwnKind();
 
----------------
This change looks to be unrelated to the patch?


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


================
Comment at: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:61
     }
-    for (unsigned Val : NonNull->args()) {
-      if (Val >= NumArgs)
+    for (ParamIdx Idx : NonNull->args()) {
+      if (Idx.getASTIndex() >= NumArgs)
----------------
`const ParamIdx &`


https://reviews.llvm.org/D43248