[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Michael Buch via lldb-commits]

Michael137 wrote:

> "completing (getting the definition) of the method's arguments and/or return types", or something else? My understanding is that we're currently always adding the declaration of the methods when completing the class, but that we don't eagerly complete the types arguments or results of the methods (except for covariant methods).

Sorry I was a bit too loose in my terminology. You're correct, LLDB currently adds all methods to the containing class, and it's pretty cheap, because we don't fully complete arguments/return types. With this patch it becomes expensive when the ASTImporter gets involved. The core idea here is that we no longer do minimal import for records. So when we get to importing a class, the ASTImporter imports all the methods that LLDB attached, which in turn import all the parameters and return types, fully. That's where type explosion comes in.

> completing all argument/result types seems rather unnecessary (I don't think C++ requires that for anything) and very expensive.

True, and AFAICT the ASTImporter has no notion of that atm (I'd need to confirm this again), so I played around with the idea of not adding all methods to the containing class, and instead add it when the Clang parser asks us (which is tricky to do well because we usually don't know that we're parsing a method call). Another idea was to add more control over which types the ASTImporter imports fully, though I never got a great implementation going for that.

> Might I ask why? Was it too unwieldy? I can certainly see how a change like this would be hard to keep under a flag, but on the other hand, a risky/wide-sweeping change like this is where a flag would be most useful (e.g., it would enable incremental implementation&bugfixes instead of revert-reapply Yoyo-ing)

It isn't too bad, but having two implementations of this mechanism seemed like a maintenance burden (e.g., runs the risk of not being removed in the long run, need to run the test-suite with and without the setting, etc.). If the community is open to it, I don't mind. Incremental bugfixing/improvements is a big plus. I guess it might be easier to decide on this once we ironed out the remaining TODOs. For reference, here's what it looks like with the setting: https://github.com/apple/llvm-project/pull/8222 (approximately 50 references to `UseRedeclCompletion()` when I check my local branch)

https://github.com/llvm/llvm-project/pull/95100