rsmith added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4320
+ if (!RD->hasDefinition())
+ return APValue();
APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
----------------
hokein wrote:
> sammccall wrote:
> > This doesn't look all that safe - you're using a `None` value to indicate
> > failure, but no current code path does that and none of the callers seem to
> > check for failure.
> > (e.g. `evaluateVarDecl` returns true instead of false).
> > Presumably we're going to get a diagnostic somewhere (though it's not
> > completely obvious to me) but can we be sure we won't assume this value has
> > the right type somewhere down the line?
> >
> > I get the feeling this is correct and I don't have enough context to
> > understand why... how about you :-)
> I don't have a promising explanation neither.
>
> I didn't find a better way to model failures in `getDefaultInitValue`. This
> function is used in multiple places of this file (and I'm not sure whether we
> should change it).
>
> @rsmith any thoughts?
`APValue()` is a valid representation for an object of class type, representing
a class object that is outside its lifetime, so I think it's OK to use this
representation, if we can be sure that this only happens along error paths.
(It's not ideal, though.)
If we can't be sure this happens only along error paths, then we should produce
a diagnostic here. Perhaps feed an `EvalInfo&` and a source location into every
caller of this function and produce a diagnostic if we end up querying the
default-initialized value of an incomplete-but-valid class type. Or perhaps we
could check that the class is complete and valid from every caller of this
function instead. (I think that we guarantee that, for a valid complete class
type, all transitive subobjects are of valid complete types, so checking this
only once at the top level before calling into `getDefaultInitValue` should be
enough.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80981/new/
https://reviews.llvm.org/D80981
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
