[PATCH] D139107: [clangd] Allow to build Clangd without decision forest
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 5 02:56:07 PST 2022
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
More information about the cfe-commits
mailing list