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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:436
   case CXXConstructExpr::CK_Complete: {
+    assert(CE && "Complete constructors cannot be inherited!");
     std::tie(State, Target) =
----------------
NoQ wrote:
> 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).
Ok. `assert(CE && !CIE && "A complete constructor is inherited!");` sounds good to me :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D74735