rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
I'm surprised so few setters needed to be made public for this to work. If this
required making lots more setters public, I'd be concerned, because we
generally don't want large chunks of the AST to be mutable after creation. But
this seems fine.
================
Comment at: clang/include/clang/AST/DeclBase.h:629-630
+public:
+ void setFromASTFile() { FromASTFile = true; }
+
----------------
aprantl wrote:
> rsmith wrote:
> > Setting this after creating a `Decl` is not safe in general; declarations
> > with this flag set have 8 bytes of additional storage allocated before
> > their start.
> >
> > An `assert(FromASTFile || hasLocalOwningModuleStorage());` would at least
> > catch the unsafe cases. This also needs a comment saying that it's not safe
> > to use in general, and perhaps a scarier name to discourage people from
> > calling it. (Unless you're creating declarations with
> > `::CreateDeserialized` -- in which case you'd need all the `Decl`
> > subclasses to befriend the creator, making this function unnecessary --
> > this is currently only going to be safe if `ModulesLocalVisibility` is
> > enabled.)
> I think the warning is unnecessary, since the storage is always allocated.
> Please let me know if I misunderstood something.
The storage isn't necessarily allocated if the decl is created with `::Create`
rather than `::CreateDeserialized`. Just changing the comment to say the
declaration must have been created with `CreateDeserialized` would be fine.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75560/new/
https://reviews.llvm.org/D75560
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
