erichkeane marked 5 inline comments as done.
erichkeane added a comment.
In D64914#1591744 <https://reviews.llvm.org/D64914#1591744>, @aaron.ballman
wrote:
> There seems to be some separable concerns in this patch. I'd rather real with
> `warn_unused_result`, `const`, and `pure` attributes separately only because
> those are under the `gnu` attribute namespace and I'd prefer to match GNU's
> semantics for those attributes (or at least understand better where the
> inconsistencies are).
>
> This is also missing documentation changes and the updated feature test macro
> value.
I'll remove the const/pure from this, I was intending to do it for completeness
and in anticipation of GNU changing their implementation to handle all 3, but I
dont have reason to believe besides a 'hunch' that they will as well. As far
as the warn_unused_result and gnu::warn_unused_result spellings, ParsedAttr
doesn't seem to have a good way to separate them. Suggestions?
I'll update the documentation as well with an example of this being used on a
type/constructor. The feature test macro (__has_attribute value) wasn't in the
paper, and it was only altering a note, so I wasn't sure, but seeing as
nodiscard has its own flag, I'll do so.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2833-2835
if (D->getFunctionType() &&
- D->getFunctionType()->getReturnType()->isVoidType()) {
+ D->getFunctionType()->getReturnType()->isVoidType() &&
+ !isa<CXXConstructorDecl>(D)) {
----------------
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?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:288-295
+ if (const Attr *A = Ctor->getAttr<PureAttr>()) {
+ Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+ return;
+ }
+ if (const Attr *A = Ctor->getAttr<ConstAttr>()) {
+ Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+ return;
----------------
aaron.ballman wrote:
> GCC does not warn on these cases and does not let you write the attributes on
> a constructor. Do you know if they're intending to implement this?
I do not know about pure/const, so I've removed them.
Repository:
rC Clang
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
