Style guide proposal: Recommend noexcept for move constructors

[Brett Wilson]

Currently the C++11 style guide says:

Chromium compiles without exception support, but there are still cases where explicitly marking a function as noexcept may be necessary to compile, or for performance reasons. Usage should be rare.

I would like to suggest this be changed to:

Chromium compiles without exception support, but there are still cases where explicitly marking a function as noexcept may be necessary to compile, or for performance reasons. Use noexcept for move constructors whenever possible (see "Notes" section on move constructors). Other usage should be rare.

Brett

[dan...@chromium.org]

You had a bunch of problems trying to use it on the flat_set but I wasn't following along closely why. Are there platform-specific problems this would encounter with our current toolchains, if we were trying to use this all the time?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV-GYQ7d2Jggbi3Mmsfacxycuyu462VkzwripoAA%2BT_5OQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Mar 24, 2017 at 1:04 PM, Brett Wilson <bre...@chromium.org> wrote:

Chromium compiles without exception support, but there are still cases where explicitly marking a function as noexcept may be necessary to compile, or for performance reasons. Use noexcept for move constructors whenever possible (see "Notes" section on move constructors). Other usage should be rare.

Is it feasible to blanket-change this in the existing codebase?  Such a change would also presumably smoke out any toolchain bugs.

PK 

[Daniel Cheng]

We should also investigate is how noexcept interacts with = default if the definition and declaration are split, e.g.:

foo.h

class Foo {

  Foo(Foo&&) noexcept;
};

foo.cc

Foo::Foo(Foo&&) noexcept = default;

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8i4tMqNpHhg/fsPKHVxGAQAJ is a previous thread that mentioned this (it looked like the problem was with some Android bots)

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCOGqB0onW4p3XyhfJB0fqkq1-burMP%3DyOO_KcqB6PckQ%40mail.gmail.com.

[Brett Wilson]

My change was because this noexcept move constructor calls a nutty internal thing with perfect forwarding that derives from the value-comparator and something breaks. This code is also shared by some non-noexcept cases. It's probably possible to write but this properly but this is beyond my current template knowledge.

Normal move constructors seem fine. I don't think the flat_map thing is a red flag.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQibP%2BtiTQVq%2BKH3NU3oCuEXr2Tw054qZ%2BwD7Nqo1dW0w%40mail.gmail.com.

[Brett Wilson]

"noexcept = default" at least works in GN:

https://cs.chromium.org/chromium/src/tools/gn/value.cc?l=76

But this is probably not being compiled on Android.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrUsz_vE-yEAamzD-JCX3Ka%3Dr2UTexzOKUKKeMUfpb4SQ%40mail.gmail.com.

[Jeremy Roman]

Assuming that this works on all of our compilers, I have no objection to using "noexcept" (as with -fno-exceptions, we want to call std::terminate anyhow). I suspect getting copies instead of moves in std::vector is a very common pitfall (since vector only moves if there's no copy constructor or the type is nothrow move constructible).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV8Ek98UadJJVtw0DGFY2ZuXg14r0q42e6MG97%2BM_tYwvQ%40mail.gmail.com.

[dan...@chromium.org]

On Mon, Mar 27, 2017 at 1:44 PM, Jeremy Roman <jbr...@chromium.org> wrote:

Assuming that this works on all of our compilers, I have no objection to using "noexcept" (as with -fno-exceptions, we want to call std::terminate anyhow). I suspect getting copies instead of moves in std::vector is a very common pitfall (since vector only moves if there's no copy constructor or the type is nothrow move constructible).

These are my feelings as well, perhaps it's as easy as making a CL that will run on all platforms that adds noexcept to some move constructors that are =default.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13e-_sdaGd-gfLzMOYJFHJjupqcQ4k_dfC3VbDCEbEyYCg%40mail.gmail.com.

[Brett Wilson]

I'll try doing this for base::FilePath.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaS331pYV%3DBnrH7hCW6_LgyZxsGekRVokrmiBy_vMZJMaw%40mail.gmail.com.

[Jeremy Roman]

It seems like at least the move assignment operator is rejected on two of our platforms (though maybe the move constructor would work, as I'd expect that to be complained about first):

MSVC gives this failure on Brett's patch:
FAILED: obj/base/base/file_path.obj ninja -t msvc -e environment.x86 -- E:\b\c\goma_client/gomacc.exe "E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe" /nologo /showIncludes /FC @obj/base/base/file_path.obj.rsp /c ../../base/files/file_path.cc /Foobj/base/base/file_path.obj /Fd"obj/base/base_cc.pdb" e:\b\c\b\win\src\base\files\file_path.cc(194): error C2610: 'base::FilePath &base::FilePath::operator =(base::FilePath &&) noexcept': is not a special member function which can be defaulted e:\b\c\b\win\src\base\files\file_path.cc(194): note: exception specification does not match the implicitly declared specification.

And on Android:

../../base/files/file_path.cc:194:11: error: function 'base::FilePath& base::FilePath::operator=(base::FilePath&&)' defaulted on its redeclaration with an exception-specification that differs from the implicit declaration 'base::FilePath& base::FilePath::operator=(base::FilePath&&)' FilePath& FilePath::operator=(FilePath&& that) noexcept = default;

[Brett Wilson]

The move constructor is fine, only the specific pattern "operator=(&&) noexcept = default" is a problem for Android and Windows.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13e3mGwAmN4FqHVb2LJK0d1psFd3XFkwBKMn%2BHGwBy2VyQ%40mail.gmail.com.

[Daniel Cheng]

Is having a noexcept move constructor sufficient to always use the vector fast path?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV-OURQjf14EyPEy5UXXpw50-DZtTkvf5zxXOwZdMoJ2KA%40mail.gmail.com.

[Brett Wilson]

According to this:

http://en.cppreference.com/w/cpp/container/vector/push_back

"If T's move constructor is not noexcept and T is not CopyInsertable into *this, vector will use the throwing move constructor. If it throws, the guarantee is waived and the effects are unspecified."

So I think vector only cares about the move constructor.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKq0Yf70JENM8OG4vfdv8FAZ51v5C70uBB4unf%2BYpKwaJg%40mail.gmail.com.

[Brett Wilson]

Here is the proposed change:

https://codereview.chromium.org/2782973002/

I did not mention the problems with "noexcept = default" assignment operators. I can, but that seemed unusual enough and the section was already on the long side for the column format we have.

Brett

[dan...@chromium.org]

On Wed, Mar 29, 2017 at 3:37 PM, Brett Wilson <bre...@chromium.org> wrote:

Here is the proposed change:

https://codereview.chromium.org/2782973002/

I did not mention the problems with "noexcept = default" assignment operators. I can, but that seemed unusual enough and the section was already on the long side for the column format we have.

Does vector benefit from noexpect assignment operators, or is this intentionally only calling out constructors?

[Jeremy Roman]

On Wed, Mar 29, 2017 at 3:40 PM, <dan...@chromium.org> wrote:

On Wed, Mar 29, 2017 at 3:37 PM, Brett Wilson <bre...@chromium.org> wrote:

Here is the proposed change:

https://codereview.chromium.org/2782973002/

I did not mention the problems with "noexcept = default" assignment operators. I can, but that seemed unusual enough and the section was already on the long side for the column format we have.

Does vector benefit from noexpect assignment operators, or is this intentionally only calling out constructors?

My understanding is that it's only construction (using std::move_if_noexcept).

[Brett Wilson]

On Wed, Mar 29, 2017 at 12:44 PM, Jeremy Roman <jbr...@chromium.org> wrote:

On Wed, Mar 29, 2017 at 3:40 PM, <dan...@chromium.org> wrote:

On Wed, Mar 29, 2017 at 3:37 PM, Brett Wilson <bre...@chromium.org> wrote:

Here is the proposed change:

https://codereview.chromium.org/2782973002/

I did not mention the problems with "noexcept = default" assignment operators. I can, but that seemed unusual enough and the section was already on the long side for the column format we have.

Does vector benefit from noexpect assignment operators, or is this intentionally only calling out constructors?

My understanding is that it's only construction (using std::move_if_noexcept).

This is my understanding as well.

Brett

[Brett Wilson]

To follow up on this, I tried to add move constructors in a few new places and noticed some bugs on some Android bots. It seems to be around "noexcept = default" being based on certain data members in the class.

This works:

  class Foo {

   public:

    Foo(Foo&&) noexcept;

   private:

    std::basic_string<char> text_;

  };

  Foo::Foo(Foo&&) noexcept = default;

Replacing the text_ declaration with a base::char16 (=uint16_t) fails:

    std::basic_string<base::char16> text_;

Primitive type members and custom classes with explicit nothrow move constructors work., std::vector doesn't. Somehow these containers get confused and either don't detect properly that their contained types are nothrow move constructible, or they don't pass on this information to the compiler properly.

I spent a while trying to trace down why basic_string<char> works but basic_string<uint16_t> fails and didn't get anywhere. It looks like basic_string uses

  std::is_nothrow_move_constructible<std::allocator<Type>>::value

for the noexcept declaration on the basic_string move constructor, and as far as I can tell this results in "true" (correct).

Do we think we might upgrade our STL on Android any time soon?

I still think we should be recommending this. If you bother to write a move constructor, you expect it to be called from vectors and such. However, it also seems super mysterious. How about appending to the description:

Note: implementing noexcept move constructors with "=default" if your class contains certain STL members won't work on Android. If you get an error you will need to write it out.

Brett

[Brett Wilson]

Or we could just say "...noexcept = default" wont work in some cases (bug).

[Avi Drissman]

There's movement on the NDK front, though maybe not in the direction we'd want.

If you recall, John Budorick helped me cherrypick a noexcept fix from upstream into the NDK. That just got reverted and my question of why was ignored.

Maybe if we're lucky it was interfering with a roll and we'll get full compatibility soon, but I have no idea.

Avi

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV_82VwjXfKVHsq675ojD3XHdwt1-f5mc-kSWNMoCYuqfQ%40mail.gmail.com.

[Chris Blume]

If we REALLY want a now-and-future-compatible way, you could use conditional noexcept.
But that might become so noisy so quickly that I don't think I would want it outside of a library.

class Foo {

   public:

    Foo(Foo&&) noexcept(std::is_nothrow_move_constructible<decltype(text_)>);

   private:

    std::basic_string<char> text_;

  };

  Foo::Foo(Foo&&) noexcept(std::is_nothrow_move_constructible<decltype(text_)>) = default;

I am all for this verbosity in libraries. But for the typical "declare your move ctors noexcept"...probably not.

Chris Blume | Software Engineer | cbl...@google.com | +1-614-929-9221

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbNtLwYm%3D%3DTiRy6yskQD9G7_Mg0%2Bk2p66%3D6APn3cdwWCQ%40mail.gmail.com.

[Avi Drissman]

(This turns out to be me mis-reading PolyGerrit, where it seems that some random gmail person clicked the "revert" button. Why do we have that switch?)

[dyar...@yandex-team.ru]

Typically this type of noexcept deduction is only for user supplied types.

I don't think that it's reasonable to deduce noexcept for concrete types you depend on for however deep library code.

Noexcept is part of the interface and it's OK to know what the interface is.

Best,

Denis.

четверг, 30 марта 2017 г., 23:44:19 UTC+3 пользователь Chris Blume написал:

[Daniel Cheng]

noexcept seems to (more or less) work now, so I think we can probably just recommend it universally where it makes sense now.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/de594284-cfed-47e8-b640-3bc6ac9d1a97%40chromium.org.

[dyar...@yandex-team.ru]

Wait, is the operator=(&&) noexcept = default issue fixed or not?

Best,

Denis

четверг, 3 мая 2018 г., 20:47:23 UTC+3 пользователь Daniel Cheng написал:

[Daniel Cheng]

It worked for me. I changed a few things and no builds seemed to break: https://crbug.com/706963

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/e9d5a093-7a93-4e64-a97e-5d282fc6a2e4%40chromium.org.

[dyar...@yandex-team.ru]

You did move constructor but not move assignment (according to what I know - you really should).

As far as I understood this conversation the main issue was an assignment operator.

Best,

Denis.

пятница, 4 мая 2018 г., 11:51:09 UTC+3 пользователь Daniel Cheng написал:

[Jan Wilken Dörrie]

The external Google Style guide got updated recently and includes the following passage:

"""

Specify noexcept when it is useful and correct.

...

You can assume that noexcept on move constructors has a meaningful performance benefit.

""" [1]

So I don't believe there is an explicit need to update the Chromium style guide anymore, but if we want to demand noexcept on all move constructors of user defined types that are used in containers I'm all for it. 

From what I know noexcept move constructors are enough to not trigger copies during vector's resize (i.e. move_if_noexcept<T> evaluates to T&&), a noexcept move assignment operator is not strictly necessary. Do you have more info here, dyaroshev@?

[1] https://google.github.io/styleguide/cppguide.html#noexcept

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/7f67a37a-13c4-4772-a2c3-35e2824adec3%40chromium.org.

[Daniel Cheng]

The Android bots seem to be fine with noexcept move assignment operators that are defined out of line with = default: https://chromium-review.googlesource.com/c/chromium/src/+/1044531

That being said, there's a weird NaCl build issue with noexcept move constructors (and move assignment operators) that I haven't gotten around to debugging... but if your code doesn't build in NaCl, then it should be fine.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALB5StZeDYy4Eqk4scHSfXzKSRET5xKReKT4dsbRCNuT_94d8g%40mail.gmail.com.

[dyar...@yandex-team.ru]

I did an experiment: https://chromium-review.googlesource.com/c/chromium/src/+/1046005

Everything works! Horray!

пятница, 4 мая 2018 г., 20:35:57 UTC+3 пользователь Daniel Cheng написал:

[Yuri Wiitala]

So, I'm sure I'll forget to do this every single time. Is there going to be a clang-presubmit to remind us to do this?

Also, out of curiosity, if we're turning off exceptions in our builds, how can using noexcept have any impact on performance? Is it some kind of "compiler semantics" thing where, for example, resolving some expressions would result in the copy constructor being called instead of the move constructor?

[Chris Blume]

Compiling with exceptions disabled does something unintuitive. For example, it doesn't prevent exceptions from happening in your program.

I am a big fanboy of exceptions and would love to nerd out about this. But to keep the story short(-ish) and get to your question:

In C++11 you can now check if a function is marked as noexcept at compile time.
void foo() noexcept;
void bar() noexcept(foo());
That first line says foo() is noexcept.
The second line says bar() is noexcept if the expression "foo()" is itself noexcept.

This means you can have a function take code path A if noexcept is true and take code path B if noexcept is false.
std::vector does exactly this.

If std::vector needs to reallocate to make more space (say you are calling push_back on the vector), it will copy each element from the old storage to the new storage. If element 10 throws an exception during copy, the vector can safely fall back to its old storage.

However, imagine std::vector used move instead of copy. If it moved elements from the old storage to the new storage and element 10's move threw an exception, we wouldn't know if it is safe to move elements 0-9 back. Those might throw as well. And maybe element 10 cannot be recovered. There is no graceful way to handle an exception here from vector's point of view.

So vector checks at compile time if T's move constructor throws. If not, it uses the code path which moves elements. Otherwise, it copies elements.




A bit of extra detail: There has been talk about having noexcept added to all functions which are compiled with exceptions disabled. This goes back to the unintuitive nature and how exceptions can still happen.
void foo() {

  bar();

}

Imagine bar() is a library call. Looking at just this piece of code we don't know if bar(); will throw or not. It might.
The C++ standard says when it does we should propagate that exception out of foo(), since we didn't catch it there.

If I compile my code (foo) with the automatic noexcept inserted:
void foo() noexcept {

  bar();

}
The standard says this should do something different.
This now needs to call std::terminate() because the noexcept was violated.

Here in Chrome those effectively result in the same thing: crashing the process. But changing your program's behavior isn't something the compiler will do by default. So then the discussion became "Maybe we can pass a compiler flag", like fast math does. And that then mutated into "Why not just use the flag which disabled exceptions?" IIRC that is the latest and it is looking more and more desirable.

Chris Blume | Software Engineer | cbl...@google.com | +1-614-929-9221

[Yuri Wiitala]

Thanks for the explanation, Chris. Interesting interactions in both the compile-time evals and run-time evals.

FWIW, from a "user of the compiler" perspective, I'd love it if "disable exceptions" also meant "the whole program is noexcept" since any syntax related to generating or handling exceptions is really just noise to me.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPHsw36NQX%2BJCDQJpJ3k3bA23yCOPyf1p7Sv64EQoeikkOSk3A%40mail.gmail.com.

[Peter Kasting]

On Fri, May 11, 2018 at 6:03 PM Yuri Wiitala <m...@chromium.org> wrote:

FWIW, from a "user of the compiler" perspective, I'd love it if "disable exceptions" also meant "the whole program is noexcept" since any syntax related to generating or handling exceptions is really just noise to me.

My proposal on this (which is the thread I think Chris is referring to) was basically that, when compiling with exceptions disabled, the compiler should treat all conditional noexcepts as if they return true.  That is, functions are not actually marked noexcept (and thus do not need to add extra "catch exception and terminate" blocks), but code which asks if other functions throw exceptions is always told that they do not (so vector and similar code always take fast paths).

This would give us the perf wins of noexcept move constructors without the perf costs of (manual or automatic) noexcept or the hassle of manual noexcept.

PK

[dyar...@yandex-team.ru]

Hi!

Another unforeseen problem with noexcept move operations: standard containers are not required to be nothrow_move_constructible.

For example, in MSVC std::set(std::set&&) is not marked noexcept (even conditionally) => therefor anything that has std::set as a member (like DeleteInfo) and has a defaulted noexcept move constructor, breaks MSVC build.

Do we care about MSVC?

Best,

Denis

пятница, 24 марта 2017 г., 23:04:55 UTC+3 пользователь Brett Wilson написал:

Currently the C++11 style guide says:

Chromium compiles without exception support, but there are still cases where explicitly marking a function as noexcept may be necessary to compile, or for performance reasons. Usage should be rare.

I would like to suggest this be changed to:

Chromium compiles without exception support, but there are still cases where explicitly marking a function as noexcept may be necessary to compile, or for performance reasons. Use noexcept for move constructors whenever possible (see "Notes" section on move constructors). Other usage should be rare.

Brett

[dyar...@yandex-team.ru]

In my opinion, a reasonable solution could be to walk back requiring to declare move operations out of line:

If you write type(type&&) noexcept = default - from what I've seen, it works everywhere regardless of whether the members are noexcept move constructible/assignable (haven't look at the standardize, maybe it's not guarantied).

Best,
Denis

вторник, 22 мая 2018 г., 13:01:44 UTC+3 пользователь dyar...@yandex-team.ru написал:

[Daniel Cheng]

I'm not against this, as long as it's demonstrated that this doesn't have a negative impact on binary size.

Ideally, the toolchain would make inlining decisions for us and we wouldn't have to think about it at all but we're not there yet...

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/41c2c588-9cb3-4f77-80e5-cda040a6b493%40chromium.org.

[dan...@chromium.org]

On Tue, May 22, 2018 at 1:59 PM Daniel Cheng <dch...@chromium.org> wrote:

I'm not against this, as long as it's demonstrated that this doesn't have a negative impact on binary size.

+1. AFAICT this doesn't break our key function strategy, as anything large enough to make clang-plugin require out-of-line things will also require a destructor, which can act as such.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKoA8J6tusSr7n7s0imSMTjafK74PNB1a%3Df%2Bmnw7oaKN%3DA%40mail.gmail.com.

[dyar...@yandex-team.ru]

I created a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=848478