[PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/Attr.td:1384
@@ +1383,3 @@
+def RequireConstantInit : InheritableAttr {
+  let Spellings = [GCC<"require_constant_initialization">,
+                   CXX11<"clang", "require_constant_initialization">];
----------------
Providing both seems fine, but regardless this should be a GNU spelling not a GCC spelling, because GCC doesn't support the attribute.

================
Comment at: include/clang/Basic/AttrDocs.td:836-840
@@ +835,7 @@
+  let Content = [{
+This attribute specifies expectations about the initialization of static and
+thread local variables. Specifically that the variable has a
+`constant initializer <http://en.cppreference.com/w/cpp/language/constant_initialization>`_
+according to the rules of [basic.start.static]. Failure to meet this expectation
+will result in an error.
+
----------------
This reads a bit awkwardly, since you don't actually say what the attribute does until the second sentence. Maybe fold the first two sentences together:

"This attribute specifies that the variable to which it is attached is intended to have a constant initializer according to the rules of [basic.start.static]. The variable is required to have static or thread storage duration. If the initialization of the variable is not a constant initializer, an error will be produced."

================
Comment at: include/clang/Basic/AttrDocs.td:842
@@ +841,3 @@
+
+Static objects with constant initializers avoid hard-to-find bugs caused by
+the indeterminate order of dynamic initialization. They can also be safely
----------------
Static objects -> Static storage duration variables

================
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.
+
----------------
static constructors -> dynamic initializers?

================
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
----------------
OK even though T has a non-trivial destructor? This makes the variable unsafe to use during program shutdown in the general case.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2575
@@ -2574,3 +2574,3 @@
   "functions, variables, classes, and Objective-C interfaces|"
-  "Objective-C protocols|"
+  "Objective-C protocols|variables with static or thread-local storage duration|"
   "functions and global variables|structs, unions, and typedefs|structs and typedefs|"
----------------
thread-local -> thread, per standard terminology

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6842
@@ +6841,3 @@
+def err_require_constant_init_failed : Error<
+  "variable does not have a constant initializer">;
+
----------------
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.

================
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)
----------------
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.

================
Comment at: test/SemaCXX/attr-require-constant-initialization.cpp:7-15
@@ +6,11 @@
+
+#if !__has_feature(cxx_static_assert)
+# define CONCAT_(X_, Y_) CONCAT1_(X_, Y_)
+# define CONCAT1_(X_, Y_) X_ ## Y_
+
+// This emulation can be used multiple times on one line (and thus in
+// a macro), except at class scope
+# define static_assert(b_, m_) \
+  typedef int CONCAT_(sa_, __LINE__)[b_ ? 1 : -1]
+#endif
+
----------------
Just use `_Static_assert`.

================
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
+}
----------------
Did you mean for this to still be here?


https://reviews.llvm.org/D23385