[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 2 inline comments as done.
NoQ added a comment.

In D74735#1880617 <https://reviews.llvm.org/D74735#1880617>, @baloghadamsoftware wrote:

> In D74735#1880390 <https://reviews.llvm.org/D74735#1880390>, @xazax.hun wrote:
>
> > If the AST is hard to work with would it make sense to try to change the AST a bit?
>
>
> Hmm, you mean making `CXXConstructExpr` an abstract base with subclasses `CXXSimpleConstructExpr` and `CXXInheritedConstructExpr`? For me it seems reasonable but I am afraid it would be a huge impact to clang, and not only to the analyzer.


I'm not extremely confident of my ability to write down the argument expressions correctly (which is the bigger problem here), especially when arguments need to be constructed. Like, not sure if it's harder to get right than this patch, but it's definitely easier to get wrong.



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:436
   case CXXConstructExpr::CK_Complete: {
+    assert(CE && "Complete constructors cannot be inherited!");
     std::tie(State, Target) =
----------------
martong wrote:
> baloghadamsoftware wrote:
> > martong wrote:
> > > Should there be rather `CIE` in the assert? Or the text after `&&` is confusing. 
> > Surely not `CIE`, since the code below uses `CE`. I see nothing confusing here: `CE` must exist because complete constructors cannot be inherited, thus `CIE` cannot exist.
> Maybe it's just me and seems like nit picking, but this is still confusing for me b/c above `CK` gets its value either from `CE` or from `CIE`. And the case depends on the kind of `CK`.
> 
> Perhaps it would be more expressive (?) : `assert(!CIE && CE && "Inherited constructors cannot be complete!");`
> Alternatively maybe the assert above in line 400 could be `assert(!CE != !CIE);` to express that it is either inherited or not but cannot be both (logical xor).
The big question here is, should assert messages explain what happens when they fail, or why shouldn't they fail? I.e., `assert(CE && !CIE && "A complete constructor is inherited!");` or `assert(CE && !CIE && "A complete constructor cannot be inherited!");`? I think it's the former because that produces a more understandable crash dump. In this sense, i agree that the assert is confusing.

> Alternatively maybe the assert above in line 400 could be `assert(!CE != !CIE);`

Dunno, that's kinda obvious because they're `dyn_cast`s to unrelated classes (and thankfully we don't have much multiple inheritance in the AST).


================
Comment at: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp:30
+
+namespace arguments_with_constructors {
+struct S {
----------------
baloghadamsoftware wrote:
> Not `constructors_with_arguments`?
The focus is on how `B::B(S, int)` has an argument of type `S` that needs to be constructed. So it's kinda `constructors_with_arguments_with_constructors`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D74735