aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
The attribute itself is looking reasonable aside from some minor nits, but this
should not be committed until the clang-tidy functionality is approved (since
it has no utility beyond clang-tidy).
================
Comment at: include/clang/Basic/Attr.td:96
+ [{!S->isStatic() && !S->isConst()}],
+ "non-static non-const member functions">;
+
----------------
mboehme wrote:
> aaron.ballman wrote:
> > `non-static, non-const member functions` (with the comma)
> IIUC, Clang then translates the comma into an "and" -- so the actual
> diagnostic becomes "non-static and non-const member functions" (see the
> expected-error in the tests). Is this as intended?
Ugh, that's a deficiency in ClangAttrEmitter.cpp. Good thing diagnostics aren't
grammatically correct anyway, you can roll back to the previous form. Sorry
about the churn!
================
Comment at: include/clang/Basic/Attr.td:2945
+def Reinitializes : InheritableAttr {
+ let Spellings = [CXX11<"clang", "reinitializes">];
+ let Subjects = SubjectList<[NonStaticNonConstCXXMethod], ErrorDiag>;
----------------
mboehme wrote:
> aaron.ballman wrote:
> > I think it makes sense to use `CXX11` instead of `Clang` as this attribute
> > doesn't make sense in C2x mode, but should there be a `GNU` spelling as
> > well?
> I have to admit I'm not sure. What are the usual considerations here? Does
> this need to be coordinated in any way with GNU, or can Clang simply
> introduce additional "GNU-style" spellings (i.e. with `__attribute__`) that
> are Clang-only?
>
> I understand there's a difference between `GCC` spellings and `GNU` spellings
> -- but I'm not sure what the rules around the latter are. Let me know if you
> think this should have a `GNU` spelling, and I'll add it!
We generally want things with both spellings unless there's rationale
justifying why only one spelling is better. I don't see any reason why this
shouldn't have a GNU spelling as well.
I'd spell this as `Clang<"reinitializes", 0>` so it gets both the CXX11 and GNU
spellings but not the C2x spelling.
================
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``reinitializes`` attribute can be applied to a non-static, non-const C++
+member function to indicate that this member function reinitializes the entire
----------------
mboehme wrote:
> aaron.ballman wrote:
> > While I kind of understand the restriction on a `const` member function,
> > what about code that has mutable members being reset within a const method?
> I find it hard to envision a method that reinitializes an object by modifying
> only mutable member variables. Mutable members are, after all, intended to be
> used for purposes (such as caching and locking) that do not change the
> "logical state" of the object, whereas reinitialization is an operation that
> does change the logical state of the object.
>
> If it really does turn out that there are legitimate use cases for applying
> the 'reinitializes' attribute to a const member function, we can always relax
> this requirement later on.
That makes sense to me, thank you for the explanation.
Repository:
rC Clang
https://reviews.llvm.org/D49911
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
