dblaikie added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:14524
+ VD->hasAttr<UnnamedAddrAttr>() &&
+ (!VD->getType().isPODType(getASTContext()) ||
+ !VD->getType().isConstQualified())) {
----------------
aeubanks wrote:
> rnk wrote:
> > I don't think `isPODType` really describes what we want. I think we want
> > `isConstantStorage` which @dblaikie just added, which means essentially,
> > can this global be placed in a readonly section.
> @dblaikie is
> [this](https://github.com/llvm/llvm-project/blob/08d7377b67358496a409080fac22f3f7c077fb63/clang/lib/AST/Type.cpp#L124)
> missing a check for a trivial constructor? using `isConstantStorage` the
> test is failing on `const Foo` by warning there when it shouldn't be
(I realize this patch has been abandoned for now (probably worth actually
marking it abandoned - I'm guessing folks are going to be wanting to look at
outstanding phab reviews to try to get them cleaned out as we transition to
github pull requests), just leaving some comments for the sake of it)
I think the `isConstantStorage` made some assumptions that the construction
issues were already handled externally to the call... or you could pass in a
bool to the second argument `ExcludeCtor` - but it's overly coarse/simplistic
and I think just says "if you're not excluding the ctor, and the type is a
CXXRecordDecl, then it's non-trivial" (so you'd pass in `false` for
`ExcludeCtor` if you know the ctor you're using is non-trivial, otherwise pass
in `true`). Because the function doesn't know which ctor you're using, so it's
up to the caller to check. The dtor is checked, however (if `ExcludeDtor` is
false).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158223/new/
https://reviews.llvm.org/D158223
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
