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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
