rsmith added inline comments.
================ Comment at: include/clang/Basic/Attr.td:301 bit DuplicatesAllowedWhileMerging = 0; + // Set to true if this attribute should apply to template declarations, + // remains false if this should only be applied to the definition. ---------------- erichkeane wrote: > rsmith wrote: > > I find this confusing -- it seems to suggest the attribute would be applied > > to the template declaration, not the templated declaration. I also think > > that the property we're modelling here is something more general than > > something about templates -- rather, I think the property is "is this > > attribute only meaningful when applied to / inherited into a defintiion?" > > It would also be useful to make clear that this only applies to class > > templates; for function templates, we always instantiate all the attributes > > with the declaration. > > > > Looking through our current attribute set, it looks like at least `AbiTag` > > should also get this set, and maybe also `Visibility`. (Though I wonder if > > there would be any problem with always instantiating the full attribute set > > for a class declaration.) > > (Though I wonder if there would be any problem with always instantiating > > the full attribute set for a class declaration.) > > This is definitely a good point. It seems that just about every other usage > of instantiating attributes happens right after creation, class template > specialization is the lone exception it seems. > > If I were to simply revert most of this change, then alter my > SemaTemplate.cpp changes to just call InstantiateAttrs and presumably remove > the other call to InstantiateAttrs (since it results in 2 sets of > attributes), would you consider that to be more acceptable? > > > Yes, I think we should at least try that and see if there's a reason why we would need the extra complexity here. https://reviews.llvm.org/D27486 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
