[PATCH] C99 partial re-initialization behavior (DR-253)

[Richard Smith]

This is looking really good. I'm not entirely sure that `NoInitExpr` is necessary -- we could define a null `Expr*` within the update list of a `DesignatedInitUpdateExpr` as meaning "perform no initialization" instead -- but we can look at removing it after this patch lands.


================
Comment at: include/clang/AST/Expr.h:4313
@@ +4312,3 @@
+  DesignatedInitUpdateExpr(const ASTContext &C, SourceLocation lbraceloc,
+                           Expr* baseExprs, SourceLocation rbraceloc);
+
----------------
Put the `*` on the right, please, and capitalize as `lBraceLoc` / `rBraceLoc`.

================
Comment at: lib/AST/Expr.cpp:2779
@@ +2778,3 @@
+      return false;
+    assert(false && "DIUE->getBase() should not be a constant expression");
+    break;
----------------
I would think this should be:

    return Base->isConstantInitializer(...) && Update->isConstantInitializer(...)

================
Comment at: lib/AST/Expr.cpp:4001
@@ +4000,3 @@
+
+  InitListExpr *ILE = new (C) InitListExpr(C, lbraceloc, None, rbraceloc);
+  ILE->setType(baseExpr->getType());
----------------
Given that we store the brace locations on the Updater, do we need to also store them inside `DesignatedInitUpdateExpr`?

================
Comment at: lib/AST/Expr.cpp:4006-4016
@@ +4005,13 @@
+
+SourceLocation DesignatedInitUpdateExpr::getLocStart() const {
+  if (LBraceLoc.isValid())
+    return LBraceLoc;
+  return getBase()->getLocStart();
+}
+
+SourceLocation DesignatedInitUpdateExpr::getLocEnd() const {
+  if (RBraceLoc.isValid())
+    return RBraceLoc;
+  return getUpdater()->getLocEnd();
+}
+
----------------
This will give a range of Base start to Updater end if there's no LBraceLoc / RBraceLoc, which is a source range that can contain things unrelated to the update. Updater's end loc is always the same as RBraceLoc anyway, so should this instead return `getBase()->getLocEnd()`?

================
Comment at: lib/AST/StmtProfile.cpp:716-724
@@ -715,1 +715,11 @@
 
+// FIXME: how to test these functions?
+void StmtProfiler::VisitDesignatedInitUpdateExpr(
+    const DesignatedInitUpdateExpr *S) {
+  llvm_unreachable("Not implemented");
+}
+
+void StmtProfiler::VisitNoInitExpr(const NoInitExpr *S) {
+  llvm_unreachable("Not implemented");
+}
+
----------------
To test these, put a `DesignatedInitUpdateExpr` in the signature of a function template, and declare it twice:

    template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'x'}))) {} // expected-note {{previous}}
    template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'r'}))) {} // ok
    template<typename T> void f(decltype(T((char[4]){"boo", [2] = 'x'}))) {} // ok
    template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'x'}))) {} // expected-error {{redefinition}}

================
Comment at: lib/CodeGen/CGExprCXX.cpp:955-972
@@ -954,1 +954,20 @@
 
+  // If we have a struct whose every field is value-initialized, we can
+  // usually use memset.
+  if (auto *ILE = dyn_cast<InitListExpr>(Init)) {
+    if (const RecordType *RType = ILE->getType()->getAs<RecordType>()) {
+      if (RType->getDecl()->isStruct()) {
+        unsigned NumFields = 0;
+        for (auto *Field : RType->getDecl()->fields())
+          if (!Field->isUnnamedBitfield())
+            ++NumFields;
+        if (ILE->getNumInits() == NumFields)
+          for (unsigned i = 0, e = ILE->getNumInits(); i != e; ++i)
+            if (!isa<ImplicitValueInitExpr>(ILE->getInit(i)))
+              --NumFields;
+        if (ILE->getNumInits() == NumFields && TryMemsetInitialization())
+          return;
+      }
+    }
+  }
+
----------------
This seems like a good idea, but not obviously related to this change. Can you split this out into a separate patch?

================
Comment at: lib/Sema/SemaInit.cpp:653-664
@@ -598,3 +652,5 @@
+      ILE->setInit(Init, ILE->getArrayFiller());
+    else if (!InitExpr && !ILE->hasArrayFiller()) {
       ExprResult ElementInit = PerformEmptyInit(SemaRef, ILE->getLocEnd(),
                                                 ElementEntity,
                                                 /*VerifyOnly*/false);

@@ -603,3 +659,7 @@
         return;
       }
 
+      Expr *Filler = FillWithNoInit ?
+          new (SemaRef.Context) NoInitExpr(ElementType) :
+          ElementInit.getAs<Expr>();
+
----------------
If we're using `NoInit` here, we shouldn't `PerformEmptyInit`, because that might fail and in any case will wastefully produce an AST node that we don't want.

================
Comment at: lib/Sema/SemaInit.cpp:2187-2204
@@ +2186,20 @@
+        StructuredList = Result;
+      else if (ExistingInit->isConstantInitializer(SemaRef.Context, false)) {
+        InitListExpr *Result = new (SemaRef.Context) InitListExpr(
+            SemaRef.Context, D->getLocStart(), None, DIE->getLocEnd());
+
+        QualType ResultType = CurrentObjectType;
+        if (!ResultType->isArrayType())
+          ResultType = ResultType.getNonLValueExprType(SemaRef.Context);
+        Result->setType(ResultType);
+
+        // If we decomposed an aggregate initializer into individual components,
+        // we do not issue diagnostics here; instead we wait till later and
+        // issue diagnostics on each individual component.
+        partialMergeFrom(SemaRef.Context, Result, ExistingInit);
+        ExistingInit = nullptr;
+        StructuredList->updateInit(SemaRef.Context, StructuredIndex, Result);
+        StructuredList = Result;
+      }
+      else {
+        if (DesignatedInitUpdateExpr *E =
----------------
I would prefer to see this case handled by teaching AST/ExprConstant to evaluate `DesignatedInitUpdateExpr` and always using the code below, rather than trying to convert an existing initializer into an `InitListExpr`.

================
Comment at: lib/Sema/SemaInit.cpp:2829-2831
@@ +2828,5 @@
+  else if (StructuredList->hasArrayFiller()) {
+    SemaRef.Diag(expr->getLocStart(),
+                 diag::warn_initializer_overrides)
+      << expr->getSourceRange();
+  }
----------------
Is it useful to warn on overriding an array filler with an explicit value? This case seems unlikely to be a bug.

================
Comment at: test/CodeGen/partial-reinitialization2.c:2
@@ +1,3 @@
+// RUN: %clang_cc1 -isystem %S/Inputs %s -emit-llvm -o - \
+// RUN:   | lli -force-interpreter | FileCheck %s
+
----------------
Executable/interpreted tests aren't acceptable here. Please check for specific IR being generated instead.

http://reviews.llvm.org/D5789

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/