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'));
----------------
ilya-biryukov wrote:
> hokein wrote:
> > 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.
> > If we receive a notification, that means we have clangd bugs.
> True, but that might happen. It'd be better to not break in that case.
> D67165 is definitely moving in the right direction, thanks!
It should be much simpler to **not** construct the
`semanticHighlightingFeature` with D67165, right?
```
if (getConfig<boolean>('semanticHighlighting')) {
const semanticHighlightingFeature =
new semanticHighlighting.SemanticHighlightingFeature();
context.subscriptions.push(
vscode.Disposable.from(semanticHighlightingFeature));
clangdClient.registerFeature(semanticHighlightingFeature);
}
```
Could we do that?
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
