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