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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein 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'));
----------------
ilya-biryukov wrote:
> 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.
> 
> How can we be sure we won't actually get any notifications?
If we receive a notification, that means we have clangd bugs. 

I understand you point here, an ideal solution is to avoid too many usages of `SemanticHighlightingFeature` in the client side, after D67165, it'd help simplify the patch here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67096