Page MenuHomePhabricator

Implement P1771
ClosedPublic

Authored by erichkeane on Jul 18 2019, 5:57 AM.

Details

Summary

EWG met on 7/18/19 and came to the conclusion that our initial intent
was to apply to constructors, so approved the paper and expected
implementers to use implementers discretion to backport this to previous
standards. ChandlerC was particularly vocal in wanting this.

The intent is to enable the library to start using this on the
constructors of scope_guard/lock_guard.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.Jul 18 2019, 5:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 5:57 AM

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.

clang/lib/Sema/SemaDeclAttr.cpp
2833–2835 ↗(On Diff #210539)

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

clang/lib/Sema/SemaStmt.cpp
284 ↗(On Diff #210539)

const auto * here and below?

288–295 ↗(On Diff #210539)

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?

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
70 ↗(On Diff #210539)

Formatting is a bit messy here.

erichkeane marked 5 inline comments as done.Jul 18 2019, 11:26 PM

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.

clang/lib/Sema/SemaDeclAttr.cpp
2833–2835 ↗(On Diff #210539)

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?

clang/lib/Sema/SemaStmt.cpp
288–295 ↗(On Diff #210539)

I do not know about pure/const, so I've removed them.

erichkeane marked an inline comment as done.

Removes pure/const, limits change to just CXX11 spelling, and changes docs/FeatureTestMacro.

aaron.ballman added inline comments.Jul 19 2019, 5:09 AM
clang/include/clang/Basic/Attr.td
2346 ↗(On Diff #210753)

I don't think this is strictly required, but perhaps it's not a bad idea.

clang/include/clang/Basic/AttrDocs.td
1499 ↗(On Diff #210753)

We should probably also talk about type conversions as well.

clang/lib/Sema/SemaDeclAttr.cpp
2833–2835 ↗(On Diff #210539)

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?

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
69 ↗(On Diff #210753)

I think this also needs tests for type conversions, from d1771r1.

87–88 ↗(On Diff #210753)

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.

erichkeane marked 2 inline comments as done.Jul 19 2019, 5:24 AM

I'll need to rebase this on another patch soon anyway, so I'll hold off until next week to update this particularly since we have some open questions. The additional TableGen work is tempting to do, though I'm not completely sure it'll get more than just this use.

I'd also like to see how this paper goes through CWG and see what the general attitude is there.

clang/lib/Sema/SemaDeclAttr.cpp
2833–2835 ↗(On Diff #210539)

An additional member of Spelling-Kind (vs Syntax kind) seems very valuable here, though I wonder if it is valuable enough of a change for TableGen. The isCXX11Attribute && !getScopeName seems a little obtuse from a readers perspective.

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
87–88 ↗(On Diff #210753)

I am unfamiliar with what Core is treating it as, but my understanding is that EWG encouraged implementers to treat it as such.

aaron.ballman added inline comments.Jul 19 2019, 5:33 AM
clang/lib/Sema/SemaDeclAttr.cpp
2833–2835 ↗(On Diff #210539)

Yeah, I would love to solve this through the tablegen interface at some point, as that solves other problems we have (like the fact that we don't AST dump with the syntax the user wrote). Let's hold off on that for now and use the obtuse approach, but with a FIXME for us to come back and touch it again once we get tablegen to track more syntactic information along with the semantic attribute object.

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
87–88 ↗(On Diff #210753)

We expose the attribute in all its glory in all language modes, so these changes already do what we want in effect. The only real question is whether we want to claim it's a C++17 extension or a C++2a extension. If a user turns on extension warnings, we should probably tell them when the feature was added, which is C++2a. It would be a bit weird to claim this is a C++17 when the feature test for it is __has_attribute(nodiscard) == 201907L (due to the normative wording changes).

But if Core moves it as a DR, then C++17 is fine, though I suppose SD-6 would need to make clear what is required for each given attribute feature test value to give us the answer.

rsmith added a subscriber: rsmith.Jul 21 2019, 4:41 AM

Please update cxx_status.html to mark P1771R1 as implemented in SVN.

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
87–88 ↗(On Diff #210753)

We moved this change as a DR, so this feature should be treated as if it were always part of the spec.

erichkeane marked an inline comment as done.

Rebased and did all the comments (including the www_status). @aaron.ballman : Wasn't positive what you meant about the conversion functions, but I think I got one? I would also love some bikeshedding on the documentation.

Rebased and did all the comments (including the www_status). @aaron.ballman : Wasn't positive what you meant about the conversion functions, but I think I got one?

I was talking about [dcl.attr.nodiscard]p2.2: "an explicit type conversion (7.6.1.8 [expr.static.cast], 7.6.3 [expr.cast], 7.6.1.3 [expr.type.conv]) that constructs an object through a constructor declared nodiscard, or that initializes an object of a nodiscard type." and specifically the part about type conversion operators that are marked nodiscard directly.

clang/include/clang/Basic/AttrDocs.td
1518 ↗(On Diff #211094)

I think we may want to add a case like:

struct S {
  operator marked_type() const;
  [[nodiscard]] operator int() const;
};

void func() {
  S s;
  static_cast<marked_type>(s); // diagnoses
  (int)s; // diagnoses
}
clang/lib/Sema/SemaDeclAttr.cpp
2835 ↗(On Diff #211094)

Conversion functions?

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
100 ↗(On Diff #211094)

If you specify a message on the nodiscard attribute used here, does the message get printed?

erichkeane marked 2 inline comments as done.Jul 23 2019, 1:53 PM

Ugg... well conversion functions are an interesting bit, as well as Type{}; with no constructors. It ends up being a function style cast to an init-list-expr, so its going to require a bit more work. Stay tuned :)

clang/lib/Sema/SemaDeclAttr.cpp
2835 ↗(On Diff #211094)

Conversion functions don't return 'void', so the 2nd condition should do it, right?

aaron.ballman marked an inline comment as done.Jul 23 2019, 1:58 PM
aaron.ballman added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
2835 ↗(On Diff #211094)

true -- we already seem to diagnose a conversion function that returns void anyway. Sorry for the noise!

Disregard that last commit... i ended up breaking stuff apparently.

Casting implementation had to happen in SemaStmt, because other 'warn unused result' stuff depends on them being the way they are.

aaron.ballman added inline comments.Jul 24 2019, 2:15 PM
clang/lib/Sema/SemaStmt.cpp
296–301 ↗(On Diff #211504)

We use this pattern in a handful of places. I wonder if we want to add a private function to Sema to handle this: void DiagnoseNodiscard(const WarnUnusedResultAttr *A, SourceLocation Loc, bool IsConstructor); (maybe it could return a partial diagnostic so we can optionally apply the source ranges to it as well?)

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
91 ↗(On Diff #211504)

Can you add a test that adds a message to the nodiscard attribute written directly on a conversion operator?

erichkeane marked 2 inline comments as done.
aaron.ballman accepted this revision.Jul 25 2019, 7:43 AM

LGTM aside from a minor naming nit.

clang/lib/Sema/SemaStmt.cpp
201 ↗(On Diff #211740)

isCtor -> IsCtor

This revision is now accepted and ready to land.Jul 25 2019, 7:43 AM
Closed by commit rL367027: Implement P1771 (authored by erichkeane, committed by ). · Explain WhyJul 25 2019, 8:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 8:12 AM