[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This seems a little short on tests as well, are there any semantic implications here that should be validated?  How does this work with function-local statics?  What does the behavior look like in C?

Else, I think @rjmccall felt the strongest about the semantics/design here, so I'd like to see him be ok with them before accepting this.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2201-2210
+  if ((CodeGenOpts.KeepStaticConsts || CodeGenOpts.KeepStaticVariables) && D &&
+      isa<VarDecl>(D)) {
     const auto *VD = cast<VarDecl>(D);
-    if (VD->getType().isConstQualified() &&
-        VD->getStorageDuration() == SD_Static)
-      addUsedOrCompilerUsedGlobal(GV);
+    if (VD->getStorageDuration() == SD_Static) {
+      if (CodeGenOpts.KeepStaticVariables)
+        addUsedOrCompilerUsedGlobal(GV);
+      else if (CodeGenOpts.KeepStaticConsts && VD->getType().isConstQualified())
----------------
hubert.reinterpretcast wrote:
> aaron.ballman wrote:
> > Reformatted with whatever clang-format does for that, as I doubt I have the formatting correct.
> @qianzhen, I don't think the suggestion was applied/applied correctly.
> 
> There should not be an `isa` followed by `dyn_cast`. That said, I think `dyn_cast_or_null` is perhaps appropriate instead of plain `dyn_cast`. Additionally, `VD` being non-null should be part of the condition.
Note `dyn_cast_or_null` is deprecated, but making the condition:

`if (const auto *VD = dyn_cast_if_present<VarDecl>(D))`

is probably the best way to simplify this here.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150221/new/

https://reviews.llvm.org/D150221