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