aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/Attr.td:182 + // it would always be false. + string DisallowImplicitThisParamName = disallowImplicitThisParamName; +} ---------------- jdenny wrote: > aaron.ballman wrote: > > jdenny wrote: > > > aaron.ballman wrote: > > > > jdenny wrote: > > > > > 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? > > > > > > > > > > > > > > > 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? > > > > > > > > That redundancy could probably be worked around in ClangAttrEmitter.cpp > > > > by inspecting the attribute argument list and noticing duplicate > > > > `[Variadic]ParamIndexArgument` before generating the code for the > > > > arguments, perhaps? > > > > > > > > > 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. > > > > > > > > This was more along the lines of what I was thinking of. > > > > > > > > Basically, it seems like the declarative part in Attr.td should be able > > > > to specify an attribute argument is intended to be a function parameter > > > > positional index, and whether implicit this needs special handling or > > > > not. Past that, the semantics of the attribute really shouldn't matter > > > > -- when asking for the index, it should be automatically adjusted so > > > > that users of the attribute don't have to do anything, and when > > > > printing the attribute, the index should be appropriately adjusted > > > > back. I'd like to avoid needing collusion between the declarative part > > > > in Attr.td and the semantic part in SemaDeclAttr.cpp because that > > > > collusion can get out of sync. It feels like we should be able to hide > > > > that collusion in ClangAttrEmitter.cpp by using some extra flags on the > > > > attribute that get set (automatically) during construction. The way you > > > > have things set up right now is very close to this, except the > > > > information has to be passed to the constructor manually in > > > > SemaDeclAttr after being calculated by > > > > checkFunctionOrMethodParameterIndex(). > > > > > > > > I'm not too worried about the memory usage at this point; if it looks > > > > problematic, we can likely address it. > > > > That redundancy could probably be worked around in ClangAttrEmitter.cpp > > > > by inspecting the attribute argument list and noticing duplicate > > > > [Variadic]ParamIndexArgument before generating the code for the > > > > arguments, perhaps? > > > > > > Seems reasonable. > > > > > > There's also the question of the constructor interface: does it accept > > > the same information multiple times or one time? > > > > > > For now, the memory optimization doesn't seem too important, so I won't > > > work on detecting duplicates. If we implement that later, we should be > > > able to easily adjust callers for the resulting interface change. > > > > > > >> 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. > > > > > > > > This was more along the lines of what I was thinking of. > > > > > > Sorry, does "This" refer to the alternative proposal I just described > > > above or your following paragraph? > > > > > > > Basically, it seems like the declarative part in Attr.td should be able > > > > to specify an attribute argument is intended to be a function parameter > > > > positional index, and whether implicit this needs special handling or > > > > not. > > > > > > Attr.td cannot specify declaratively whether implicit this needs special > > > handling (I've been calling this DisallowImplicitThisParam, but let's > > > call it AdjustForImplicitThis for clarity) because part of that decision > > > is per function: whether the function actually has an implicit this > > > (HasImplicitThis). Attr.td might always be able to specify whether > > > implicit this is allowed (AllowImplicitThis) and thus, when there > > > actually is an implicit this (HasImplicitThis), whether an extra > > > increment should be added for printing (AdjustForImplicitThis = > > > HasImplicitThis && !AllowImplicitThis). ClangAttrEmitter.cpp would then > > > have to learn how to compute AdjustForImplicitThis from the other two > > > Booleans, but SemaDeclAttr.cpp already does that. > > > > > > In my current patch, only SemaDeclAttr.cpp understands how to compute > > > AdjustForImplicitThis, but ClangAttrEmitter.cpp documents it too and > > > knows there's always another increment (to make indices one-origin) for > > > printing. In my alternative proposal above, ClangAttrEmitter.cpp would > > > not document or understand any of this. Instead, ClangAttrEmitter.cpp > > > would only know that indices must be adjusted by some arbitrary value for > > > the sake of printing, and SemaDeclAttr.cpp would tell it how much. That > > > seems like the cleanest separation. > > > > > > > Past that, the semantics of the attribute really shouldn't matter -- > > > > when asking for the index, it should be automatically adjusted so that > > > > users of the attribute don't have to do anything, and when printing the > > > > attribute, the index should be appropriately adjusted back. > > > > > > Agreed. Both my current patch and my alternative proposal above offer > > > that. > > > > > > > I'd like to avoid needing collusion between the declarative part in > > > > Attr.td and the semantic part in SemaDeclAttr.cpp because that > > > > collusion can get out of sync. > > > > > > The only collusion I see in my current patch (besides some perhaps > > > unnecessary documentation in ClangAttrEmitter.cpp) is the understanding > > > that the Boolean indicates that one increment beyond the usual single > > > increment is needed when printing. I believe my alternative proposal > > > eliminates even that: then there's just some arbitrary increment when > > > printing, and ClangAttrEmitter.cpp doesn't know or care why. > > > > > > > It feels like we should be able to hide that collusion in > > > > ClangAttrEmitter.cpp by using some extra flags on the attribute that > > > > get set (automatically) during construction. The way you have things > > > > set up right now is very close to this, except the information has to > > > > be passed to the constructor manually in SemaDeclAttr after being > > > > calculated by checkFunctionOrMethodParameterIndex(). > > > > > > In my last response, I was explaining why > > > checkFunctionOrMethodParameterIndex probably needs to calculate that > > > information anyway for the sake of its users, and it seems that having > > > both SemaDeclAttr and ClangAttrEmitter know how to calculate it is worse > > > for maintenance than having SemaDeclAttr pass its result to > > > ClangAttrEmitter. My alternative proposal above does not require > > > ClangAttrEmitter to calculate the adjustment at all. It just receives > > > the net adjustment from SemaDeclAttr. > > > > > > I feel like my alternative proposal would satisfy your desire for a > > > better separation of concerns. The difference from what I think you're > > > describing is that my alternative proposal keeps the index adjustment > > > amount computation in SemaDeclAttr rather than trying to move any of that > > > computation to ClangAttrEmitter. Does that seem reasonable to you? > > > Sorry, does "This" refer to the alternative proposal I just described > > > above or your following paragraph? > > > > Your proposal, which I thought was solving what I described in the > > following paragraph. > > > > > Attr.td might always be able to specify whether implicit this is allowed > > > (AllowImplicitThis) and thus, when there actually is an implicit this > > > (HasImplicitThis), whether an extra increment should be added for > > > printing (AdjustForImplicitThis = HasImplicitThis && !AllowImplicitThis). > > > ClangAttrEmitter.cpp would then have to learn how to compute > > > AdjustForImplicitThis from the other two Booleans, but SemaDeclAttr.cpp > > > already does that. > > > > Okay, I think I see where we may not quite be on the same page. The Attr.td > > bit knows whether the attribute needs to adjust for implicit this or not, > > but only SemaDeclAttr.cpp knows about the actual function the attribute is > > trying to be applied to. What I was envisioning was to move the logic from > > SemaDeclAttr.cpp into something accessible from the code generated by > > ClangAttrEmitter.cpp, but that may not be sufficient because the semantic > > attribute needs that information from the specific Decl, which by > > definition requires something additional to be passed in from > > SemaDeclAttr.cpp. > > > > Thanks for bearing with me. :-) > > > > > I feel like my alternative proposal would satisfy your desire for a > > > better separation of concerns. The difference from what I think you're > > > describing is that my alternative proposal keeps the index adjustment > > > amount computation in SemaDeclAttr rather than trying to move any of that > > > computation to ClangAttrEmitter. Does that seem reasonable to you? > > > > I think it's reasonable, depending on how it's surfaced. Would it be > > reasonable to make it look something like this? > > ``` > > // Attr.td > > // ... > > let Args = [VariadicParamIndexArgument<"Args", AdjustsImplicitThis>]; > > // ... > > > > // SemaDeclAttr.cpp > > // ... > > ImplicitThisAdjustment AdjustsImplicitThis; > > if (!checkFunctionOrMethodParameterIndex(..., ..., AdjustsImplicitThis)) > > return; > > > > // ... > > D->addAttr(::new (S.Context) NonNullAttr(..., ..., AdjustsImplicitThis)); > > ``` > > (I'm using enumerations in both files rather than Boolean values so it's > > more clear what's happening). > > > > This implies that any attribute accepting a parameter index will have a > > constructor that requires you to specify whether that instance of the > > attribute adjusts implicit this or not. > > What I was envisioning was to move the logic from SemaDeclAttr.cpp into > > something accessible from the code generated by ClangAttrEmitter.cpp, but > > that may not be sufficient because the semantic attribute needs that > > information from the specific Decl, which by definition requires something > > additional to be passed in from SemaDeclAttr.cpp. > > Well, maybe we can do that. ClangAttrEmitter.cpp could generate for the > attribute a static member function that massages one index into the form > users expect. That function's parameters would be the original index and > HasImplicitThis (or maybe the function Decl itself). AllowsImplicitThis > would be declared in Attr.td as you want. > checkFunctionOrMethodParameterIndex could use that function to compute each > index before it examines it, and callers could pass that resulting index to > the attribute constructor. It seems slightly messy because users of the > attribute must know to call that function on indices before passing them to > the constructor, but it means ClangAttrEmitter.cpp contains all the index > adjustment logic, and Attr.td declares the property AllowsImplicitThis, which > I agree feels like a static property of an attribute. > > At this point, I think I'll just write a patch and see what you think. We > can adjust if it's still too ugly. > > > Thanks for bearing with me. :-) > > Likewise. It sounds like we're converging. > > At this point, I think I'll just write a patch and see what you think. We can > adjust if it's still too ugly. I think that sounds like a good idea. https://reviews.llvm.org/D43248 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
