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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
