ChuanqiXu9 wrote: > > I think we shouldn't remove the assertion. Your test passes with the > > removal of the assertion since the initializers are not complex. So it ends > > quickly. But if it is a complex initialization which triggers more > > deserialization, I feel it will be problematic. > > I think the point is in `DeclMustBeEmitted`, this should be a "pure" method > > but it triggers deserialization. > > I think, the proper solution may be: > > ``` > > 1. When we write a VarDecl, use a bit to record whether the var decl has an > > initialization with side effects. > > > > 2. When we read a var decl with the above information, let's record it in a > > set in ASTReader. > > > > 3. When we decide if a VarDecl needs to be emitted in `DeclMustBeEmitted`, > > let's lookup it in the above set. > > ``` > > Thanks for providing context! I'm not very familiar with this part of clang. > I have a few clarifying questions: `DeclMustBeEmitted` is part of > `ASTContext` rather than `ASTReader`, so unless I missed something it > wouldn't have access to this set. We could check the "has initializer with > side effect" set in `ASTReader` before the call to `DeclMustBeEmitted`, but > unless we removed that piece of code from `DeclMustBeEmitted` it would still > result in the initializer being deserialized, right? There are other callers > that would not necessarily have an`ASTReader` in context, so I'm not sure how > to go about that. > > Actually, looking at the callers, one of them is `isRequiredDecl`, which is > used to determine whether decls go into the `EagerlyDeserializedDecls` > record. I'm not fully clear on how eagerly they are deserialized, but if the > initializer is also eagerly deserialized, then maybe the `ASTReader` could > skip calling `DeclMustBeEmitted` for `VarDecl`s that aren't fully > deserialized, since they would have been in the `EagerlyDeserializedDecls` > record if `DeclMustBeEmitted` returned true. >
Semantically it is better to do this in `DeclMustBeEmitted` since this is what we're deciding. We can relate ASTContext and ASTReader by ExternalASTSource generally. We can add an interface in ExternalASTSource and call it in ASTContext and implement it in ASTReader. > Another option would be to set some bits in `VarDecl::NonParmVarDeclBits` > rather than creating a set in `ASTReader`. WDYT? Generally we don't do this, since it will add additional cost for non-module users. Although it is cheaper and simpler for module users, generally we prefer to give non-module users higher precedence. https://github.com/llvm/llvm-project/pull/143739 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
