Mordante marked an inline comment as done.
Mordante added a comment.
Thanks for the review! I'll wait with committing until @gribozavr2 had time to
review this patch and D71140 <https://reviews.llvm.org/D71140>.
================
Comment at: clang/lib/AST/CommentSema.cpp:693
+ StringRef AttributeSpelling =
+ CPlusPlus14 ? "[[deprecated]]" : "__attribute__((deprecated))";
if (PP) {
----------------
aaron.ballman wrote:
> Mordante wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > This attribute also exists with this spelling in C2x, FWIW.
> > > > > True, but unless I'm mistaken `CPlusPlus17` and `CPlusPlus2a` also
> > > > > include `CPlusPlus14`. Do you prefer a different name for the Boolean?
> > > > I'm talking about C2x, not C++2a. The name for the variable is fine,
> > > > but we should prefer `[[deprecated]]` in C2x mode to
> > > > `__attribute__((deprecated))`.
> > > >
> > > > I think the correct predicate is:
> > > > `getLangOpts().DoubleSquareBracketAttributes` -- if the user says they
> > > > want to use double-square bracket attributes, we should probably prefer
> > > > them to GNU-style attributes.
> > > Ah sorry I misread. I'll have a look at C2x. Thanks for the information.
> > `getLangOpts().DoubleSquareBracketAttributes` will not work since it
> > includes C++11, which doesn't support `[[deprecated]]`, so I will just test
> > for C++14 and C2x.
> > (I had a look at the proper syntax in C2x and found N2334 ;-))
> > getLangOpts().DoubleSquareBracketAttributes will not work since it includes
> > C++11, which doesn't support [[deprecated]], so I will just test for C++14
> > and C2x.
>
> We support `[[deprecated]]` in C++11 mode as an extension, but I suppose the
> concern there is pedantic warnings?
>
> I think what you're doing now is fine, but it suggests that we need a better
> interface to `clang::hasAttribute()` so that you can test for support through
> the same machinery as `__has_cpp_attribute`/`__has_c_attribute`. It wouldn't
> be suitable to use here because you want to know something more specific than
> it can tell you (whether the attribute can be used without triggering an
> extension diagnostic), but this seems like a reasonable utility for us to
> have someday.
My reason not to use `[[deprecated]]` in C++11 is standard compliance. The
original bug reporter was using clang-tidy to improve the code. We don't know
whether users of clang-tidy only use the clang compiler or also other compilers
which don't offer `[[deprecated]]` as C++11 extension.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71141/new/
https://reviews.llvm.org/D71141
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
