https://github.com/kadircet requested changes to this pull request.
I am sorry for my poor choice of words around `config knob`, I was trying to imply a knob like https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ClangdServer.h#L111. I don't think the knob needs to be exposed to end-users directly, only having programmatic access is fine initially. if you choose to expose it right away, i think a command line flag is the most natural way, rather than a config field. I'd rather plumb it explicitly through the interfaces of file-index/file-symbol/dex (and others), i think using `context` here is somewhat abusing the functionality. if you feel like that's too much (especially considering the size of the patch), feel free to leave a FIXME at least, to plumb it properly later on. > Only a user config option is respected, not a project config option. I think > this limitation is necessary, because the index is a global structure rather > than a per-project structure. (Concretely, in the call to the Dex constructor > that happens via BackgroundIndexRebuilder::maybeRebuild(), what path would I > pass to the context provider that gets it to look at an appropriate project > config file?) you're absolutely right here. that's the main reason i don't think config mechanism actually works here. they're mostly meaningful for per-project/file configurable features. not for global ones like index. > Only the Dex logic is conditioned on the option, not the binding of the > outgoingCalls message. This is because the binding of the message happens > fairly early, while we're processing the initialize request. At this point we > haven't loaded config files yet, and if we try, that triggers a > publishDiagnostics message for the config file, which confuses clients who > are expecting an initialize response. again this is resulting from my poor choice of words, if we treat this as part of option struct, we'll have the value at startup time and can decide what to do. https://github.com/llvm/llvm-project/pull/117673 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
