[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[David Blaikie via Phabricator via cfe-commits]

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