[clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed.

[yawanng]

Summary

"misc-noexcept-move-constructor" is better not to be issued when "-fno-exceptions" is set.

[chh]

LGTM, leave this to alexfh's approval.

[aaron.ballman]

Aside from one minor nit, LGTM

clang-tidy/misc/NoexceptMoveConstructorCheck.cpp

23

You can remove the spurious parens.

[alexfh]

IIUC, when vector<T> (for a class T that has both move and copy constructors) resizes, it will prefer move constructors, but only if they're declared noexcept. This is true even if -fno-exceptions is on. So I don't think this check should depend on -fno-exceptions.

[alexfh]

Here's a small test: https://godbolt.org/g/nNZgUb (look for T::T( and S::S( calls, adding -fno-exceptions doesn't change which constructors are called).

[FlameTop]

test/clang-tidy/misc-noexcept-move-constructor.cpp

2

Could I request that exceptions be explicitly enabled here (-fexceptions) as the test currently fails on targets which are configured with exceptions disabled as default?

[chh]

In D34002#775830, @alexfh wrote:

IIUC, when vector<T> (for a class T that has both move and copy constructors) resizes, it will prefer move constructors, but only if they're declared noexcept. This is true even if -fno-exceptions is on. So I don't think this check should depend on -fno-exceptions.

Should the compiler assume noexcept when -fno-exceptions is on?
That means move constructors should be preferred under -fno-exceptions, and this check would be unnecessary, right?

[alexfh]

In D34002#776193, @chh wrote:

In D34002#775830, @alexfh wrote:

IIUC, when vector<T> (for a class T that has both move and copy constructors) resizes, it will prefer move constructors, but only if they're declared noexcept. This is true even if -fno-exceptions is on. So I don't think this check should depend on -fno-exceptions.

Should the compiler assume noexcept when -fno-exceptions is on?
That means move constructors should be preferred under -fno-exceptions, and this check would be unnecessary, right?

The compiler doesn't assume noexcept and I heard from competent people the reasons why it shouldn't, though I can't immediately recall these reasons. I think, the patch should be reverted.

[alexfh]

test/clang-tidy/misc-noexcept-move-constructor.cpp

2

That would be the way to go, if we wanted to keep this change in. However, we need to revert it (which should also fix the issues you're talking about).

[chh]

Android source is suppressing misc-noexcept-move-constructor warnings
because -fno-exceptions is used and Android does not like to add more
exception specific code.

It's not a big deal for Android to suppress this check one way or the other.
I don't mind reverting it, just curious why a compiler cannot assume noexcept under -fno-exceptions.

[alexfh]

In D34002#776551, @chh wrote:

Android source is suppressing misc-noexcept-move-constructor warnings
because -fno-exceptions is used and Android does not like to add more
exception specific code.

As I've said, the lack of noexcept on move constructor of a type will make certain operations on STL containers of that type slower regardless of whether exceptions are turned on or off. -fno-exceptions will in no way help to recover the lost performance, so there's absolutely no reason to silence this check on the code that is otherwise not using exceptions.

Reverted in r305057.

It's not a big deal for Android to suppress this check one way or the other.
I don't mind reverting it, just curious why a compiler cannot assume noexcept under -fno-exceptions.

One reason for compiler not to treat each function as noexcept, when compiled with -fno-exceptions is that it would make it hard to link this code together with code compiled with -fexceptions without causing ODR violations.

[chh]

Thanks for providing the ODR reason for Android -fno-exceptions users to change source code or suppress this warning themselves.

[alexfh]

In D34002#776948, @chh wrote:

Thanks for providing the ODR reason for Android -fno-exceptions users to change source code or suppress this warning themselves.

I'm not sure I understand your comment. Could you clarify?

Here's an explanation about the possibility of an ODR violation in case the compiler would assume noexcept for all functions compiled with -fno-exceptions. Consider the following code:
a.h:

struct A {
  A(A&&);
  A(const A&);
  ...
};

b.cc:

#include "a.h"
#include <vector>
...
  std::vector<A> v1;
...

c.cc:

#include "a.h"
#include <vector>
...
  std::vector<A> v2;
...

Now if we compile b.cc with -fexceptions and c.cc with -fno-exceptions, and IF the compiler was changed to treat all functions in c.cc as noexcept, then the instantiation of some methods of std::vector<A> in b.cc would use the copy constructor of A, but the same methods would use the move constructor of A in c.cc. When these two were linked together, that would be an ODR violation.

[chh]

I understood the mentioned ODR issue, which is useful for users to understand the compiler choice. For -fno-exceptions users, they should change source code to work with mixed cases, or just ignore mixed cases and suppress this warning.

clang-tidy/misc/NoexceptMoveConstructorCheck.cpp

 void NoexceptMoveConstructorCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++11; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
-  if (!getLangOpts().CPlusPlus11)
+  if (!getLangOpts().CPlusPlus11 || !getLangOpts().CXXExceptions)
     return;
 
   Finder->addMatcher(

test/clang-tidy/misc-noexcept-move-constructor.cpp

-// RUN: %check_clang_tidy %s misc-noexcept-move-constructor %t
+// RUN: clang-tidy %s -checks="-*,misc-noexcept-move-constructor" -- -std=c++11 \
+// RUN:   | FileCheck %s -check-prefix=CHECK-EXCEPTIONS \
+// RUN:   -implicit-check-not="{{warning|error}}:"
+// RUN: clang-tidy %s -checks="-*,misc-noexcept-move-constructor" -- -fno-exceptions -std=c++11 \
+// RUN:   | FileCheck %s -allow-empty -check-prefix=CHECK-NONEXCEPTIONS \
+// RUN:   -implicit-check-not="{{warning|error}}:"
+
 
 class A {
   A(A &&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [misc-noexcept-move-constructor]
+  // CHECK-EXCEPTIONS: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [misc-noexcept-move-constructor]
+  // CHECK-NONEXCEPTIONS-NOT: warning:
   A &operator=(A &&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should
+  // CHECK-EXCEPTIONS: :[[@LINE-1]]:6: warning: move assignment operators should
+  // CHECK-NONEXCEPTIONS-NOT: warning:
 };
 
 struct B {
   static constexpr bool kFalse = false;
   B(B &&) noexcept(kFalse);
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [misc-noexcept-move-constructor]
+  // CHECK-EXCEPTIONS: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [misc-noexcept-move-constructor]
+  // CHECK-NONEXCEPTIONS-NOT: warning:
 };
 
 class OK {};