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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/Features.inc.in:7
 #define CLANGD_TIDY_CHECKS @CLANGD_TIDY_CHECKS@
+#define CLANGD_DECISION_FOREST @CLANGD_DECISION_FOREST@
----------------
sammccall wrote:
> we **could** include this in the feature-string too (Feature.cpp) but I think it's probably more noise than signal.
Maybe include only when it's disabled? I've added this to the commit, PTAL.


================
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
----------------
sammccall wrote:
> hmm, while reordering these: hueristics -> heuristics? :-)
I'm not sure how the reordering happened in the first place, TBH 🤔 
I kept it and fixed the typo, but also let me know if I should keep the old order instead.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:217
+#ifdef CLANGD_DECISION_FOREST
     init(CodeCompleteOptions().RankingModel),
+#else
----------------
sammccall wrote:
> 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.
I have moved it to the `.cpp` file. I think that avoiding conditionals in the preprocessor is a good thing definitely.

I also considered putting the constant into `DecisionForest.cpp` to reduce the number of files that use `#if CLANGD_DECISION_FOREST`.
But the downside is that definition of the constant gets harder to find, so I opted for keeping it in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139107