[PATCH] D50994: Add a new flag and attributes to control static destructor registration

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:1472
 
+  /// Do we need to emit an exit-time destructor for this variable?
+  bool isNoDestroy(const ASTContext &) const;
----------------
jfb wrote:
> rsmith wrote:
> > jfb wrote:
> > > This is only valid for static variables, right? It's probably better to document the function name that way, e.g. `isStaticWithNoDestroy`.
> > I think the question (and hence current function name) is meaningful for any variable, but it just happens that the answer will always be "no" for non-static variables (for now, at least). Are you concerned that people will think calls to this function are missing from codepaths that only deal with automatic storage duration variables with the current name?
> Yeah it seems like "this variable has no destructor". I guess even trivial automatic variables have a (trivial) destructor, so maybe it's not ambiguous? Up to y'all :)
If anyone has a strong preference I'd be happy either way!


================
Comment at: clang/include/clang/Basic/LangOptions.def:311
 
+LANGOPT(RegisterStaticDestructors, 1, 1, "Register C++ static destructors")
+
----------------
rsmith wrote:
> Should be a `BENIGN_LANGOPT` because it doesn't affect AST construction, only the generated code.
It does affect AST construction though, we don't mark a static VarDecl's type's destructor referenced in this mode, or check it's access (because we aren't actually using it). Do you think this is the wrong choice of semantics?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5931
+    handleSimpleAttributeWithExclusions<NoDestroyAttr, AlwaysDestroyAttr>(S, D, A);
+  }
+}
----------------
jfb wrote:
> This allows a variable with both attributes :)
> Looking at the code above it seems like having both attributes is equivalent to no-destroy.
`handleSimpleAttributeWithExclusions` handles that automagically, i.e.:
```
$ cat t.cpp
[[clang::no_destroy]] [[clang::always_destroy]] int x;
$ ~/dbg-bld/bin/clang t.cpp
t.cpp:1:25: error: 'always_destroy' and 'no_destroy' attributes are not compatible
[[clang::no_destroy]] [[clang::always_destroy]] int x;
                        ^
t.cpp:1:3: note: conflicting attribute is here
[[clang::no_destroy]] [[clang::always_destroy]] int x;
  ^
1 error generated.
```
I'll add a test to prove this though...


https://reviews.llvm.org/D50994