sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/CodeComplete.h:129 +#if defined(CLANGD_DECISION_FOREST) +# define DEFAULT_RANKING_MODEL DecisionForest ---------------- mgorny wrote: > sammccall wrote: > > 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) > This is what I originally wanted to do. However, it doesn't seem that this > code path is covered by the test suite and I have no clue how to use clangd > (or at least the test suite didn't see it crash when I added fatal asserts to > the decision forest paths), so I've decided that ensure compile-time failures > is the safer approach. When I add std::abort() to `evaluateDecisionForest` then I see lots of failed tests: ``` ******************** Failed Tests (69): Clangd :: completion-auto-trigger.test Clangd :: completion-snippets.test Clangd :: completion.test Clangd :: protocol.test Clangd Unit Tests :: ./ClangdTests/0/73 ... Clangd Unit Tests :: ./ClangdTests/9/73 Testing Time: 4.22s Unsupported: 7 Passed : 193 Failed : 69 ``` The compile-time failures may be safer in some ways, but I think this use of preprocessor is more intrusive than we'd like to to maintain to work around toolchain limitations on a platform where (AFAIK) we don't have any actual users. (i.e. even if the runtime behavior were untested, that is probably still preferable) 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
