[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] C99 partial re-initialization behavior (DR-253)
From:       Yunzhong Gao <Yunzhong_Gao () playstation ! sony ! com>
Date:       2015-04-30 22:58:39
Message-ID: 7e3e2b0ee1a6ea5e0ea8e54981bd9552 () localhost ! localdomain
[Download RAW message or body]

Thanks for reviewing this.


================
Comment at: lib/AST/Expr.cpp:2779
@@ +2778,3 @@
+      return false;
+    assert(false && "DIUE->getBase() should not be a constant expression");
+    break;
----------------
rsmith wrote:
> I would think this should be:
> 
> return Base->isConstantInitializer(...) && Update->isConstantInitializer(...)
This function may get called in the middle of the initialization process, at which \
point the updater still contains unfilled "holes". I do not really want to call \
isConstantInitialzier() on such an updater, therefore the assertion here. Hmm, since \
I am so confident that Base will not be a constant initializer, maybe I can even just \
set culprit to Base without running isConstantInitializer() on it?

================
Comment at: lib/AST/Expr.cpp:4001
@@ +4000,3 @@
+
+  InitListExpr *ILE = new (C) InitListExpr(C, lbraceloc, None, rbraceloc);
+  ILE->setType(baseExpr->getType());
----------------
rsmith wrote:
> Given that we store the brace locations on the Updater, do we need to also store \
> them inside `DesignatedInitUpdateExpr`?
I think you are right. I can get the brace locations from the updater without storing \
a duplicate copy.

================
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();
+}
+
----------------
rsmith wrote:
> 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()`? Updater's end loc is always the same as RBraceLoc \
> anyway.

Are you sure? If the RBraceLoc is not valid, then the updater's RBraceLoc is not \
valid either, so updater->getLocEnd() should return the end location of the last \
non-null initializer.

http://reviews.llvm.org/D5789

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



_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]