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:
> > > 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.
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
