Mark constructors that do not throw exceptions as noexcept #3278

AreaZR · 2022-12-09T17:53:51Z

No description provided.

StephanTLavavej · 2022-12-10T02:21:54Z

There are test failures.
We've asked you to avoid force-pushing PRs because this makes incremental review difficult. You acknowledged this and promised to stop, but it keeps happening. Can you explain why? I am genuinely puzzled.
When we mark something as noexcept beyond what's required in the Standard, we comment it as // strengthened or /* strengthened */ to note this fact.
Many of these are incorrect. For example, deque always dynamically allocates a "proxy object" in its constructors, including its move constructor, so that must not be marked as noexcept. (deque is highly unusual in that the proxy object participates in the data structure's representation, whereas for other containers it's for debug bookkeeping. vector and string are special in that the Standard was updated to require noexcept in the default/move ctors and we decided to mark them accordingly despite the debug allocation; this is a known headache with no good solutions.)
- Somewhat similarly, the tree containers (set etc.) always dynamically allocate sentinel nodes as part of their representation, so their move constructors must not be marked noexcept. I find it concerning that so many noexcepts are being added without any attempt to determine whether the implementations are actually capable of throwing; this makes auditing the PR for correctness time-consuming and risky.
Marking defaulted constructors as noexcept seems exceptionally low value; they will be noexcept if the compiler-generated code qualifies. (This can be used to enforce things - i.e. if the compiler-generated code wouldn't be noexcept, then noexcept = default; is an error - but this is rarely useful.)
- In some cases, the addition of noexcept to = default; is incorrect. For example, for _Not_fn(_Not_fn&&), the user-provided function object might have a throwing move constructor.
- Similarly, tuple's move constructor can easily throw.

While we can continue considering noexcept strengthening on a case-by-case basis, I think that this PR is trying to do far too much. I don't believe that we have the review capacity to merge this.

frederick-vs-ja · 2022-12-11T08:14:30Z

I think you should split this PR into two PRs.

One adds some missing noexcept in <future>, no /* strengthened */ is needed.
The other adds noexcept to stream types, /* strengthened */ is probably needed.

For containers, wrapping types, and defaulted functions, I think it's wrong to add noexcept.

stl/inc/future

frederick-vs-ja

I've looked into stream types. Unfortunately, perhaps we shouldn't add noexcept to any of their move constructors.

It may be OK to make most (but not all) move assignment operators noexcept. And we should also consider swap functions and other functions called by them.

frederick-vs-ja · 2022-12-17T16:01:18Z

stl/inc/spanstream

@@ -55,7 +55,7 @@ public:
 }

 basic_spanbuf(const basic_spanbuf&) = delete;
- basic_spanbuf(basic_spanbuf&& _Right)
+ basic_spanbuf(basic_spanbuf&& _Right) noexcept


For stream classes with their own buffers, unfortunately, we always copy constructs the basic_streambuf base class subobjects, and thus the move constructors can't be noexcept. Ditto for basic_meowbuf and basic_(i|o)meowstream classes.

frederick-vs-ja · 2022-12-17T16:03:43Z

stl/inc/spanstream

@@ -64,7 +64,7 @@ public:
 // N4892 [spanbuf.assign], assignment and swap
 basic_spanbuf& operator=(const basic_spanbuf&) = delete;

- basic_spanbuf& operator=(basic_spanbuf&& _Right) {
+ basic_spanbuf& operator=(basic_spanbuf&& _Right) noexcept {


Perhaps we should refactor this move assignment operator to avoid move construction.

I'm working on this in my (another) PR.

frederick-vs-ja · 2022-12-17T16:06:00Z

stl/inc/spanstream

@@ -226,7 +226,7 @@ public:
 // N4892 [ispanstream.assign], assignment and swap
 basic_ispanstream& operator=(const basic_ispanstream&) = delete;

- basic_ispanstream& operator=(basic_ispanstream&& _Right) {
+ basic_ispanstream& operator=(basic_ispanstream&& _Right) noexcept {


I think it's reasonable to strengthen exception specifications for move assignment operators.
However, you seemly forgot to do similar strengthening for swap functions (and other functions called by swap).

frederick-vs-ja · 2022-12-17T16:08:14Z

stl/inc/syncstream

@@ -106,7 +107,7 @@ public:
 _Tidy();
 }

- basic_syncbuf& operator=(basic_syncbuf&& _Right) {
+ basic_syncbuf& operator=(basic_syncbuf&& _Right) noexcept {


noexcept shouldn't be added here because emit may throw. This is LWG-3498.

frederick-vs-ja · 2022-12-17T16:15:04Z

stl/inc/istream

@@ -48,13 +48,13 @@ public:
 }

 protected:
- __CLR_OR_THIS_CALL basic_istream(basic_istream&& _Right) : _Chcount(_Right._Chcount) {
+ __CLR_OR_THIS_CALL basic_istream(basic_istream&& _Right) noexcept : _Chcount(_Right._Chcount) {


It seems unsuitable to add noexcept (ditto for basic_iostream and basic_ostream), since

this constructor calls basic_ios<...>::init,

init calls widen,

widen calls use_facet, which may throw bad_cast.

However, it seems OK to make move functions noexcept.

StephanTLavavej · 2023-01-04T22:32:06Z

We talked about this at the weekly maintainer meeting and have decided to close this PR without merging, for the reasons stated in #3278 (comment) . We remain open to considering small, targeted PRs that have been carefully thought through to be correct, such as @frederick-vs-ja's #3314.

AreaZR requested a review from a team as a code owner December 9, 2022 17:53

AreaZR force-pushed the noexcept branch 2 times, most recently from 45d780f to a6923ce Compare December 9, 2022 18:19

Mark constructors that do not throw exceptions as noexcept

fc3e7c1

AreaZR force-pushed the noexcept branch from a6923ce to fc3e7c1 Compare December 9, 2022 21:13

frederick-vs-ja reviewed Dec 11, 2022

View reviewed changes

stl/inc/future Show resolved Hide resolved

frederick-vs-ja mentioned this pull request Dec 12, 2022

Enhancements for future and shared_future #3284

Merged

frederick-vs-ja suggested changes Dec 17, 2022

View reviewed changes

This was referenced Dec 31, 2022

Strengthen exception specifications for stream types #3314

Merged

Make some special member functions of promise and packaged_task defaulted #3315

Merged

<mutex>: Scope guards for lock algorithms and exception specification strengthening #3321

Merged

StephanTLavavej closed this Jan 4, 2023

This was referenced Mar 13, 2023

Mark constructors that do not throw exceptions as noexcept #3565

Closed

Mark constructors that do not throw exceptions as noexcept #3574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark constructors that do not throw exceptions as noexcept #3278

Mark constructors that do not throw exceptions as noexcept #3278

AreaZR commented Dec 9, 2022

StephanTLavavej commented Dec 10, 2022

frederick-vs-ja commented Dec 11, 2022

frederick-vs-ja left a comment

frederick-vs-ja Dec 17, 2022

frederick-vs-ja Dec 17, 2022

frederick-vs-ja Dec 31, 2022

frederick-vs-ja Dec 17, 2022

frederick-vs-ja Dec 17, 2022

frederick-vs-ja Dec 17, 2022

StephanTLavavej commented Jan 4, 2023

Mark constructors that do not throw exceptions as noexcept #3278

Mark constructors that do not throw exceptions as noexcept #3278

Conversation

AreaZR commented Dec 9, 2022

StephanTLavavej commented Dec 10, 2022

frederick-vs-ja commented Dec 11, 2022

frederick-vs-ja left a comment

Choose a reason for hiding this comment

frederick-vs-ja Dec 17, 2022

Choose a reason for hiding this comment

frederick-vs-ja Dec 17, 2022

Choose a reason for hiding this comment

frederick-vs-ja Dec 31, 2022

Choose a reason for hiding this comment

frederick-vs-ja Dec 17, 2022

Choose a reason for hiding this comment

frederick-vs-ja Dec 17, 2022

Choose a reason for hiding this comment

frederick-vs-ja Dec 17, 2022

Choose a reason for hiding this comment

StephanTLavavej commented Jan 4, 2023