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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
