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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/Attr.td:1384
@@ +1383,3 @@
+def RequireConstantInit : InheritableAttr {
+  let Spellings = [GCC<"require_constant_initialization">];
+  let Subjects = SubjectList<[Var]>;
----------------
This should not use the GCC spelling because GCC doesn't support the attribute. Do you expect this attribute to do anything useful in C? If not, this should just be a CXX11 spelling in the clang namespace.

================
Comment at: include/clang/Basic/Attr.td:1385
@@ +1384,3 @@
+  let Spellings = [GCC<"require_constant_initialization">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [RequireConstantInitDocs];
----------------
You might want to make a custom `SubsetSubject` for this. Check out `NormalVar`, `SharedVar`, and the likes in Attr.td. Then you can get rid of the custom handling in SemaDeclAttr.cpp.

================
Comment at: include/clang/Basic/AttrDocs.td:835
@@ +834,3 @@
+  let Category = DocCatVariable;
+  let Heading = "require_constant_initialization";
+  let Content = [{
----------------
Should be no need to specify the heading manually since there's only one spelling for the attribute.

================
Comment at: include/clang/Basic/AttrDocs.td:847
@@ +846,3 @@
+
+This attribute acts an a compile time assertion that the requirements
+for constant initialization have been met. Since these requirements change
----------------
s/acts an a/acts as a

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6842
@@ +6841,3 @@
+def err_require_constant_init_var_not_static_tls : Error<
+  "'require_constant_initialization' attribute can only be applied to variables "
+  "static or thread-local storage duration">;
----------------
I would reword this to: "'require_constant_initialization' attribute only applies to variables with static or thread-local storage duration"

================
Comment at: test/SemaCXX/attr-require-constant-initialization.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions -std=c++03 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions -std=c++11 %s
----------------
This file should also get tests that ensure we properly diagnose the attribute when it doesn't apply to a declaration (such as a local variable, as well as nonsense like a function), test that it does not accept arguments, etc.

================
Comment at: test/SemaCXX/attr-require-constant-initialization.cpp:4-6
@@ +3,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions -std=c++14 %s
+//
+// Tests for "expression traits" intrinsics such as __is_lvalue_expr.
+//
+
----------------
Does this comment actually apply?


https://reviews.llvm.org/D23385