erichkeane wrote:
> I'm glad to see alignment here. Thank you. I didn't expect such a great
> interest. In this case, I'd raise another point (but without expanding the
> current PR):
>
> In the `CodeGenAction.cpp` we have this code:
>
> ```c++
> if (CodeGenOpts.ClearASTBeforeBackend) {
> LLVM_DEBUG(llvm::dbgs() << "Clearing AST...\n");
> // Access to the AST is no longer available after this.
> // Other things that the ASTContext manages are still available, e.g.
> // the SourceManager. It'd be nice if we could separate out all the
> // things in ASTContext used after this point and null out the
> // ASTContext, but too many various parts of the ASTContext are still
> // used in various parts.
> C.cleanup();
> C.getAllocator().Reset(); // <--- here
> }
> ```
>
> And this raises me a question, why is `C.getAllocator().Reset();` not done
> part of `cleanup` already? IMO it's very suspicious.
>
> What I suspected was that it is used from `ASTContext::PrintStats` thus
> reseting it would have an effect on debuggability. However, this
> `ASTContext::PrintStats` is called from `clang::ParseAST` after
> `Consumer->HandleTranslationUnit(S.getASTContext());` is called. This means
> that even today, this allocator is conditionally reset. So I really have no
> idea why it's not in `cleanup`; but I think I'll leave it for the experts to
> judge.
Resetting the allocator is a pretty expensive operation, so we choose not to
(and prefer to just 'leak' or continue allocation) unless we opt-in. This is
one of those things that has significant compile-time implications.
https://github.com/llvm/llvm-project/pull/191697
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
