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