jdenny marked 3 inline comments as done.
jdenny added a comment.




================
Comment at: include/clang/Basic/Attr.td:182
+  // it would always be false.
+  string DisallowImplicitThisParamName = disallowImplicitThisParamName;
+}
----------------
jdenny wrote:
> aaron.ballman wrote:
> > Is there much benefit to forcing the attribute author to pick a name for 
> > this? It seems like this is more of a Boolean value: either implicit this 
> > is allowed or not. It would be really nice if we could hide these mechanics 
> > as implementation details so that the user only needs to write 
> > `VariadicParamIndexArgument<"name", DisallowImplicitThis>` (or something 
> > similar) when defining the attribute, and ideally not have to do any extra 
> > work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if 
> > possible.
> Thanks for the review.  I'll work on that.
> Is there much benefit to forcing the attribute author to pick a name for 
> this? It seems like this is more of a Boolean value: either implicit this is 
> allowed or not.

If the attribute author picks the name, then the attribute author can ensure 
there's only one of these per attribute.  I could rewrite it to have one per 
VariadicParamIndexArgument and one per ParamIndexArgument if you like.  In that 
case, ArgumentWithTypeTagAttr would end up with two of these, and future 
attributes could potentially have more, but they should all have the same value 
within a single attribute.  I didn't investigate how that redundancy would 
actually impact memory usage.  What do you think?

>  It would be really nice if we could hide these mechanics as implementation 
> details so that the user only needs to write 
> VariadicParamIndexArgument<"name", DisallowImplicitThis> (or something 
> similar) when defining the attribute, and ideally not have to do any extra 
> work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if 
> possible.

So far, I haven't found a good way to accomplish that, or maybe I've 
misunderstood you....

The logic of checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp seems 
pretty tightly coupled with its users' logic.  For example, handleNonNullAttr 
uses the indices as adjusted by checkFunctionOrMethodParameterIndex to 
determine which indices belong in the array to be passed to the NonNullAttr 
constructor.  We could try to have NonNullAttr (in the constructor, presumably) 
perform the adjustment of indices so that SemaDeclAttr.cpp doesn't need that 
logic, but then it would be too late to for handleNonNullAttr to filter them.

The only extra work this patch adds to SemaDeclAttr.cpp beyond what's already 
there is to reuse the DisallowImplicitThis that is essentially already computed 
in checkFunctionOrMethodParameterIndex.

Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index 
adjustment logic and pass an index offset to attribute constructors, so 
ClangAttrEmitter.cpp would just know it has to print indices with some given 
offset.  But a general index offset is wider than the single bool being stored 
now.  Again, I haven't investigated the actual impact on memory usage.

Do you see a better way to achieve the encapsulation you're looking for?




================
Comment at: lib/Sema/SemaDeclAttr.cpp:355-357
   }
+  else if (DisallowImplicitThisParam)
+    *DisallowImplicitThisParam = false;
----------------
aaron.ballman wrote:
> jdenny wrote:
> > aaron.ballman wrote:
> > > Formatting is off here -- the `else if` should go up a line.
> > I don't follow.  It looks right to me. 
> Our coding standard is to write:
> ```
> if (foo) {
> } else if (bar) {
> }
> ```
> rather than
> ```
> if (foo) {
> }
> else if (bar) {
> }
> ```
> One good way to catch this sort of thing is to run your patch through 
> clang-format: 
> https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
Thanks.


https://reviews.llvm.org/D43248



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to