[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:54
+  list(APPEND COMPLETIONMODEL_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/CompletionModel.cpp)
+  list(APPEND COMPLETIONMODEL_SOURCES decision-forest/DecisionForest.cpp)
+else() 
----------------
this muddies the waters a bit: the code in DecisionForest.cpp is *not* the generated completion model.
as mentioned below, I think it's a little clearer to have this be unconditionally a source file, and #ifdef the contents.


================
Comment at: clang-tools-extra/clangd/Features.inc.in:7
 #define CLANGD_TIDY_CHECKS @CLANGD_TIDY_CHECKS@
+#define CLANGD_DECISION_FOREST @CLANGD_DECISION_FOREST@
----------------
we **could** include this in the feature-string too (Feature.cpp) but I think it's probably more noise than signal.


================
Comment at: clang-tools-extra/clangd/decision-forest/DecisionForest_disabled.cpp:19
+
+DecisionForestScores
+evaluateDecisionForest(const SymbolQualitySignals &Quality,
----------------
As much as I dislike the preprocessor, I think conditionally including source files is undiscoverable enough (if you're reading the source code) that I'd have a preference for this being an `#else` branch in DecisionForest.cpp.

(It's also one mechanism vs two, given that we *do* still need the preprocessor elsewhere)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:215
+           clEnumValN(CodeCompleteOptions::Heuristics, "heuristics",
+                      "Use hueristics to rank code completion items")),
+#ifdef CLANGD_DECISION_FOREST
----------------
hmm, while reordering these: hueristics -> heuristics? :-)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:217
+#ifdef CLANGD_DECISION_FOREST
     init(CodeCompleteOptions().RankingModel),
+#else
----------------
CodeCompleteOptions().RankingModel is also the one used in unit tests.
I think the default value in the struct should be ifdef'd, or if we **really** want to avoid pp-conditionals in header files, we could have an `Auto` value which acts sa the platform-default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139107