EricWF marked 7 inline comments as done.
================
Comment at: include/clang/Basic/AttrDocs.td:844
@@ +843,3 @@
+the indeterminate order of dynamic initialization. They can also be safely
+used by other static constructors across translation units.
+
----------------
rsmith wrote:
> static constructors -> dynamic initializers?
Changed "by other static constructors" -> "during dynamic initialization"
================
Comment at: include/clang/Basic/AttrDocs.td:858
@@ +857,3 @@
+ };
+ SAFE_STATIC T x = {42}; // OK.
+ SAFE_STATIC T y = 42; // error: variable does not have a constant initializer
----------------
rsmith wrote:
> OK even though T has a non-trivial destructor? This makes the variable unsafe
> to use during program shutdown in the general case.
Right, but I want this attribute to be able to work with (A) the union trick
for "trivial" destructors and (B) variables not used during shutdown.
I was planning on following this up with an additional feature to aid in the
shutdown case as well, but I think there is value in separating the features.
Currently -Wglobal-destructors will still warn on that declaration, so at least
the unsafe shutdown is not silently missed.
Does this behavior make sense to you?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6842
@@ +6841,3 @@
+def err_require_constant_init_failed : Error<
+ "variable does not have a constant initializer">;
+
----------------
rsmith wrote:
> It may be useful for this diagnostic to say why we consider this to be an
> error (that is, mention that there is a `require_constant_initialization`
> attribute attached to the variable). The attribute will typically be hidden
> behind a (perhaps unfamiliar to the reader) macro, so it may not be obvious
> if we don't point it out.
"variable does not have a constant initializer as required by
'require_constant_initializer' attribute"?
Or do we want the diagnostic to point to the attribute token in a "required
from here"-like note?
================
Comment at: lib/Sema/SemaDecl.cpp:10519-10520
@@ +10518,4 @@
+ auto *CE = dyn_cast<CXXConstructExpr>(Init);
+ bool DiagErr = (var->isInitKnownICE() || (CE &&
CE->getConstructor()->isConstexpr()))
+ ? !var->checkInitIsICE() : !checkConstInit();
+ if (DiagErr)
----------------
rsmith wrote:
> Falling back to `checkConstInit` here will suppress the warning on some cases
> that are not technically constant initializers (but that Clang can emit as
> constants regardless). Is that what you want? If so, you should update the
> documentation to say that instead of saying that we only check for a constant
> initializer.
> Falling back to checkConstInit here will suppress the warning on some cases
> that are not technically constant initializers (but that Clang can emit as
> constants regardless). Is that what you want?
Not really. I would prefer this strictly conform to the standard so it can be
used to portably detect possible dynamic initializers on other toolchains.
What would the correct fallback here be?
================
Comment at: test/SemaCXX/attr-require-constant-initialization.cpp:96-100
@@ +95,7 @@
+ ATTR static const int& temp_init = 42;
+#if 0
+ /// FIXME: Why is this failing?
+ __thread const int& tl_init = 42;
+ static_assert(__has_constant_initializer(tl_init), "");
+#endif
+}
----------------
rsmith wrote:
> Did you mean for this to still be here?
No.
https://reviews.llvm.org/D23385
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
