[PATCH] D67096: [clangd][vscode] Add a flag to enable semantic highlighting in clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:113
   const semanticHighlightingFeature =
-      new semanticHighlighting.SemanticHighlightingFeature();
+      new semanticHighlighting.SemanticHighlightingFeature(
+          getConfig<boolean>('semanticHighlighting'));
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > Why not avoid calling `clangdClient.registerFeature` instead?
> > > > Would allow to:
> > > > - not change the `SemanticHighlightingFeature` class, keeping it simpler,
> > > > - ensure we do not do any extra work if the feature is disabled.
> > > good point, done.
> > Should we update other uses of `semanticHighlightingFeature` too?
> > 
> > `context.subscriptions.push(vscode.Disposable.from(semanticHighlightingFeature))` probably ensures we call `dispose()` when the `clangdClient` is getting removed, I guess we definitely don't need that.
> > 
> > Other uses as well:
> > - no need to pass notification is highlighting is disabled.
> > - no need to cleanup if highlighting is disabled.
> > 
> > Maybe assign null or undefined to `semanticHighlightingFeature` when the flag is false?
> > At each usage we can check whether the `semanticHighlightingFeature` is not null and only call relevant methods if that's the case.
> I don't think it is worth updating all usages, it is no harm to keep them here even when the highlighting is disabled (the dispose is a no-op; we never receive notifications from clangd); and it would add more guard code which I'd avoid.
How can we be sure that nothing bad is going to happen?
In particular, we are "binding" notification handling, but never registered a feature. How can we be sure we won't actually get any notifications?

If we don't create the object in the first place, we are confident nothing harmful can be done with it.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67096/new/

https://reviews.llvm.org/D67096