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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:52
+  list(APPEND COMPLETIONMODEL_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/CompletionModel.cpp)
+  add_definitions(-DCLANGD_DECISION_FOREST=1)
+endif()
----------------
Rather than `add_definitions`, could you instead add a line to `clangd/Features.inc.in`, and `#include Features.h` where needed?

There are probably tradeoffs between these approaches, but that's the one we're using for other optional features.

We also use `#if` rather than `#if defined()` which ensures we're not forgetting to include a definition and getting some default behavior.


================
Comment at: clang-tools-extra/clangd/CodeComplete.h:129
 
+#if defined(CLANGD_DECISION_FOREST)
+#  define DEFAULT_RANKING_MODEL DecisionForest
----------------
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)


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

https://reviews.llvm.org/D138520