[PATCH] D138520: [clangd] Make decision forest model optional

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.h:129
 
+#if defined(CLANGD_DECISION_FOREST)
+#  define DEFAULT_RANKING_MODEL DecisionForest
----------------
sammccall wrote:
> this approach feels a bit heavy on the preprocessor, especially for a header.
> It makes the decision-forest-less version seem "complete" but at the cost of having to understand two variants of the code.
> 
> what about:
>  - keeping all the existing APIs
>  - using `#if` to change the default value of `RankingModel`, but otherwise leaving this struct unchanged
>  - providing an alternate definition of `evaluateDecisionForest` that just calls `std::abort()`
>  - having `ClangdMain` exit with an error if DecisionForest is selected and not enabled (or simply ignoring the flag)
This is what I originally wanted to do. However, it doesn't seem that this code path is covered by the test suite and I have no clue how to use clangd (or at least the test suite didn't see it crash when I added fatal asserts to the decision forest paths), so I've decided that ensure compile-time failures is the safer approach.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138520/new/

https://reviews.llvm.org/D138520