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