Page MenuHomePhabricator

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

Authored by yawanng on Jun 7 2017, 10:52 AM.

Diff Detail

Event Timeline

yawanng created this revision.Jun 7 2017, 10:52 AM
chh edited edge metadata.Jun 7 2017, 11:03 AM
chh added a subscriber: cfe-commits.

LGTM, leave this to alexfh's approval.

aaron.ballman accepted this revision.Jun 7 2017, 11:47 AM
aaron.ballman added a subscriber: aaron.ballman.

Aside from one minor nit, LGTM

clang-tidy/misc/NoexceptMoveConstructorCheck.cpp
23

You can remove the spurious parens.

This revision is now accepted and ready to land.Jun 7 2017, 11:47 AM
yawanng updated this revision to Diff 101834.Jun 7 2017, 3:37 PM
yawanng marked an inline comment as done.
yawanng closed this revision.Jun 7 2017, 3:39 PM
alexfh edited edge metadata.Jun 8 2017, 12:24 AM

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.

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 added inline comments.
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 added a comment.Jun 8 2017, 9:48 AM

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 added a comment.Jun 8 2017, 3:24 PM
In D34002#776193, @chh 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 added inline comments.Jun 8 2017, 3:27 PM
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 added a comment.Jun 8 2017, 4:30 PM

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.

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 added a comment.Jun 9 2017, 9:24 AM

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

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 added a comment.Jun 9 2017, 10:27 AM

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.