rsmith 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:
> 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?
================
Comment at: clang/include/clang/Basic/Attr.td:2999
+
+def NoDestroy : InheritableAttr {
+ let Spellings = [Clang<"no_destroy", 0>];
----------------
jfb wrote:
> Ditto for these, I think the names of the variables should clarify that it's
> only for `static`. Note that I wouldn't rename the attributes themselves!
> Usage in context is unambiguous, and the docs should be clear. I just think
> the variable names don't provide the required context.
Our convention is that the `Attr` name matches the source spelling, so I think
this is appropriate as-is.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1813
+def err_destroy_attr_on_non_static_var : Error<
+ "%select{no_destroy|always_destroy}0 attribute must be applied to a"
+ " variable with static or thread storage duration">;
----------------
"must" -> "can only". (There is no requirement to apply the attribute to
anything.)
================
Comment at: clang/include/clang/Basic/LangOptions.def:311
+LANGOPT(RegisterStaticDestructors, 1, 1, "Register C++ static destructors")
+
----------------
Should be a `BENIGN_LANGOPT` because it doesn't affect AST construction, only
the generated code.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2769-2771
+ Opts.RegisterStaticDestructors =
+ Args.hasFlag(OPT_fcxx_static_destructors, OPT_fno_cxx_static_destructors,
+ Opts.RegisterStaticDestructors);
----------------
`-fc++-static-destructors` is not a CC1 flag, so this should just be a `hasArg`
call.
Repository:
rC Clang
https://reviews.llvm.org/D50994
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
