[PATCH] D138520: [clangd] Make decision forest model optional

[Sam McCall via Phabricator via cfe-commits]

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