[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[André Schackier via Phabricator via cfe-commits]

AMS21 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:45-49
+    const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>();
+    const Expr *NoexceptExpr = ProtoType->getNoexceptExpr();
+    if (NoexceptExpr) {
+      NoexceptExpr = NoexceptExpr->IgnoreImplicit();
+      if (!isa<CXXBoolLiteralExpr>(NoexceptExpr)) {
----------------
PiotrZSL wrote:
> woudn't getExceptionSpecInfo() != EST_NoexceptFalse do a trick here ?
I've tested it and it seem that a struct like this:

```
struct B {
  static constexpr bool kFalse = false;
  B(B &&) noexcept(kFalse);
};
```

Also has the the `ExceptionSpecType` set to `EST_NoexceptFalse`. So changing this would change the previous behavior.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:95
+                                     DefaultableMemberKind Kind,
+                                     SkipMethods SkipMethods) {
+  if (!RecordDecl)
----------------
PiotrZSL wrote:
> Can we hit some endless recursion here ?
> Maybe so protection against checking Record that we currently checking.
Yes we can hit an infinite recursion here.
Take this class:

```
struct A {
    A(A &&) = default;
};
```

Since the move constructor is defaulted, we need to call `analyzeUnresolvedOrDefaulted`. There we determine the kind of defaulted member function which in this case is a move constructor.
Then we call `analyzeRecord`. Without setting `SkipMethods::Yes` we would try to check the move constructor of the class which in this case is `A`. We would call `analyze` on the move constructor of `A` which is exactly where we started and we'd have an infinite loop.

That is why I've added the `SkipMethods` parameter. While analyzing the defaulted move constructor of `A` we only want to look at its base classes and it's fields and determine if they are declared as throwing or not. While for their the base classes and fields of `A` we also want to check their move constructor (if they have any).

I hope my explanation was at least a bit helpful.

There might be a better solution to this, Before this I had essentially the same code twice for checking the bases and field and wanted to combine them.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:95
+                                     DefaultableMemberKind Kind,
+                                     SkipMethods SkipMethods) {
+  if (!RecordDecl)
----------------
AMS21 wrote:
> PiotrZSL wrote:
> > Can we hit some endless recursion here ?
> > Maybe so protection against checking Record that we currently checking.
> Yes we can hit an infinite recursion here.
> Take this class:
> 
> ```
> struct A {
>     A(A &&) = default;
> };
> ```
> 
> Since the move constructor is defaulted, we need to call `analyzeUnresolvedOrDefaulted`. There we determine the kind of defaulted member function which in this case is a move constructor.
> Then we call `analyzeRecord`. Without setting `SkipMethods::Yes` we would try to check the move constructor of the class which in this case is `A`. We would call `analyze` on the move constructor of `A` which is exactly where we started and we'd have an infinite loop.
> 
> That is why I've added the `SkipMethods` parameter. While analyzing the defaulted move constructor of `A` we only want to look at its base classes and it's fields and determine if they are declared as throwing or not. While for their the base classes and fields of `A` we also want to check their move constructor (if they have any).
> 
> I hope my explanation was at least a bit helpful.
> 
> There might be a better solution to this, Before this I had essentially the same code twice for checking the bases and field and wanted to combine them.
Another way to defiantly have an infinite recursion is if to resolve a `struct A;` we needed to resolve `struct B;` and to resolve that we needed to resolve `struct A`.  You would need something like `A` inheriting from `B` and `B` inheriting from `A` or having the other type as a member variable. Same goes for a struct which contains itself.

But I'm currently unaware on how to create such a scenario with legal C++.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:108-114
+  BasesToVisit Bases = isConstructor(Kind)
+                           ? BasesToVisit::VisitPotentiallyConstructedBases
+                           : BasesToVisit::VisitAllBases;
+
+  if (Bases == BasesToVisit::VisitPotentiallyConstructedBases)
+    Bases = RecordDecl->isAbstract() ? BasesToVisit::VisitNonVirtualBases
+                                     : BasesToVisit::VisitAllBases;
----------------
PiotrZSL wrote:
> I'm not sure if we need to be so picky...
> In short we could check all bases.
> Virtual, Abstract or not...
Honestly I not sure about it. I just tried to copy what `Sema` does when resolving noexcept.
Removing it definitely makes the code easier to read.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h:23
+public:
+  enum class State : std::int8_t {
+    Throwing,    ///< This function has been declared as possibly throwing.
----------------
PiotrZSL wrote:
> No need for std::int8_t, and if you really want then use std::uint8_t
I prefer to not use it, but saw `ExceptionAnalyzer` use it. So I just copied it from there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146922/new/

https://reviews.llvm.org/D146922