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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
