aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2346
+ // spellings.
+ bool IsCXX11NoDiscard() const {
+ return this->getSemanticSpelling() == CXX11_nodiscard;
----------------
I don't think this is strictly required, but perhaps it's not a bad idea.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1499
+marked with ``[[nodiscard]]`` or a constructor of a type marked
+``[[nodiscard]]`` will also diagnose.
+
----------------
We should probably also talk about type conversions as well.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
if (D->getFunctionType() &&
- D->getFunctionType()->getReturnType()->isVoidType()) {
+ D->getFunctionType()->getReturnType()->isVoidType() &&
+ !isa<CXXConstructorDecl>(D)) {
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > I'm not certain about this -- we do not allow you to put
> > `__attribute__((warn_unused_result))` on a constructor declaration, and
> > neither does GCC. Do you know if they're planning to change their behavior
> > as well? https://godbolt.org/z/kDZu3Q
> I'm unsure about GCC's intent, though they will likely be changing the
> [[nodiscard]] behavior due to the vote (and I doubt they will have an "if
> C++20" test either.
>
> Do we have a good way to exclude the alternate spellings? isCXX11Attribute
> only checks syntax, not the actual spelling. ParsedAttr doesn't seem to even
> contain a way to get which namespace it is associated with either. Is
> something like that valuable to add?
The way we currently do this is `(isCXX11Attribute() || isC2xAttribute()) &&
!getScopeName()`, but I am not opposed to going with an additional member. Do
you have a preference either way?
================
Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:69
+ [[gnu::warn_unused_result]] S(double);
+ };
+
----------------
I think this also needs tests for type conversions, from d1771r1.
================
Comment at: clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp:87-88
// expected-warning@28 {{use of the 'nodiscard' attribute is a C++17
extension}}
+// expected-warning@66 {{use of the 'nodiscard' attribute is a C++17
extension}}
+// expected-warning@71 {{use of the 'nodiscard' attribute is a C++17
extension}}
#endif
----------------
Is Core treating this paper as a DR? I don't have a strong opinion here, but
for the nodiscard with a message version, I made it a C++2a extension. I don't
have a strong opinion, but I sort of prefer doing whatever Core decides.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64914/new/
https://reviews.llvm.org/D64914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
