[cfe-commits] r63242 - in /cfe/trunk: Driver/RewriteObjC.cpp include/clang/AST/Decl.h include/clang/AST/Expr.h include/clang/AST/ExprCXX.h include/clang/Basic/DiagnosticSemaKinds.def lib/AST/Expr.cpp lib/AST/StmtPrinter.cpp lib/CodeGen/CGExprAgg.

[Douglas Gregor]

On Jan 28, 2009, at 2:55 PM, Eli Friedman wrote:

> On Wed, Jan 28, 2009 at 1:54 PM, Douglas Gregor <dgregor at apple.com>  
> wrote:
>> +      // Implicitly-generated value initializations are okay.
>> +      if (isa<CXXZeroInitValueExpr>(Exp->getInit(i)) &&
>> +          cast<CXXZeroInitValueExpr>(Exp->getInit(i))->isImplicit())
>> +        continue;
>> +
>>      if (CheckForConstantInitializer(Exp->getInit(i),
>>                                      Exp->getInit(i)->getType()))
>>        return true;
>
> This doesn't look quite right... I'm pretty sure a
> CXXZeroInitValueExpr should be a valid constant initializer even if it
> isn't in an init list.

I don't see this anywhere in C++ [expr.const]p2, which states the  
conditions under which an expression that is not an integral constant  
expression will be considered a constant expression for initialization.


> In any case, this seems like a massive abuse of CXXZeroInitValueExpr,
> and I'm not completely sure that the C++ rules match what you want
> here.  I'd suggest a new expression type to represent the holes.

You caught me. Yes, it should be a new expression type.

>> +      SemaRef->Diag(D->getStartLocation(),
>> +                    diag::warn_designator_into_union_broken_init)
>> +        << SourceRange(D->getStartLocation(), DIE->getSourceRange 
>> ().getEnd());
>
> If we're going to do the wrong thing, this should be an error.

CodeGen will fail, but semantic analysis is correct, and many  
important clients don't use CodeGen. That said, I tried (and failed)  
to break CodeGen, and after looking through it a little more I believe  
that my follow-on patch solves the issue for real.

>> +    SemaRef->Diag(D->getEllipsisLoc(),
>> +                  diag::warn_gnu_array_range_designator_unsupported)
>> +      << SourceRange(D->getLBracketLoc(), D->getRBracketLoc());
>
> If we're going to do the wrong thing, this should be an error.

As above, we do the semantic analysis (almost) properly, so it's  
mainly a CodeGen issue.
Also as above, my follow-on patch implements this feature *almost*  
completely (except for our handling of side-effects), so I've changed  
the warning accordingly.

>> +    // Here, xs[0].a == 0 and xs[0].b == 3, since the second,
>> +    // designated initializer re-initializes the whole
>> +    // subobject [0], overwriting previous initializers.
>> +    SemaRef->Diag(InitRange.getBegin(),  
>> diag::warn_subobject_initializer_overrides)
>> +      << InitRange;
>> +    SemaRef->Diag(ExistingInit->getSourceRange().getBegin(),
>> +                  diag::note_previous_initializer)
>> +      << ExistingInit->hasSideEffects(SemaRef->Context)
>> +      << ExistingInit->getSourceRange();
>
> There's one somewhat dangerous case here; take the following example
> at file scope:
>
> struct X { int a, b; };
> struct X xs[] = { [0] = (struct X) { 1, 2 }, [0].b = 3 };
>
> In gnu89 mode in gcc, the init list is actually equivalent to "{1,
> 3}".  clang, on the other hand, will give different results (although
> it will also warn, so it might not be so bad).

I'm not strongly inclined to model this behavior, since both Clang and  
GCC get the answer { 0, 3} in their default invocation modes, and we  
do complain about this pretty clearly. Plus, I can't actually think of  
a decent way to do it, except perhaps to break the compound literal  
down into an initializer list in gnu89 mode (but not in gnu99/c99  
mode!).


>> +bool Expr::hasSideEffects(ASTContext &Ctx) const {
>> +  EvalResult Result;
>> +  Evaluate(Result, Ctx);
>> +  return Result.HasSideEffects;
>> +}
>
> I don't think this will work quite correctly; you probably want
> something like "return !Evaluate(Result, Ctx) ||
> Result.HasSideEffects"

Yeah, it isn't really working as I'd hoped. The heuristic you give  
will work in some cases, but of course there are many expressions that  
are not constant but don't have side effects. I should probably rip  
out this method for now.

> In general, though, this does seem like a good approach.

Thanks for the review!

	- Doug