[Lldb-commits] [PATCH] D47235: Move ModuleList's dependency on clangDriver into Host

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Moving the assertion into `ClangModulesDeclVendor` seems like a good idea to me. If that's the only place that actually requires the value to be non-empty, then it should be the thing asserting it (we can put a helpful message in the assert statement telling the user what he needs to do). But, we need to be careful there to make sure that the user cannot trip that assertion by manually setting the value of the setting to "".

**However**, what would be an even better idea in my mind is to completely move the setting into the ClangExpressionParser plugin. I don't want to sound like a broken record, but I feel like noone has responded to that proposal of mine, positively or negatively. I'll try to elaborate on it more.

I think this could be modelled the same way as SymbolFileDWARF injects the `plugin.symbol-file.dwarf.comp-dir-symlink-paths` setting. The flow there is:

- SystemInitializerFull calls SymbolFileDWARF::Initialize
- SymbolFileDWARF::Initialize calls PluginManager::RegisterPlugin, passing it a DebuggerInitialize function
- when a Debugger object is created, it calls PluginManager::DebuggerInitialize  (from Debugger::InstanceInitialize)
- PluginManager::DebuggerInitialize calls SymbolFileDWARF::Initialize, passing it the debugger instance
- SymbolFileDWARF::Initialize calls PluginManager::CreateSettingForSymbolFilePlugin which creates the actual setting
- finally, when SymbolFileDWARF wants to read the setting it calls SymbolFileDWARF::GetGlobalPluginProperties()->GetSymLinkPaths()

this is all very enterprise-y, but it allows us to keep the knowledge of the comp-dir-symlink-paths setting (even it's existence) completely hidden inside the plugin, which I think is what this discussion was all about in the first place.

So my question is, is there a reason a flow like this would not work for this setting as well?


https://reviews.llvm.org/D47235