hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:134 + // The decoration datatypes used last time highlight was called. + private oldDecorations: DecorationRangePair[] = []; + // Apply decorations to the textEditor with uri and remove the old ones. ---------------- I'm not convinced that we need this interface. With the current implementation (create new TextEditorDecorationTypes every time we do a re-highlighting), we'd need to save the old decorations in order to dispose them when applying new decorations. but if we create the TextEditorDecorationTypes at Initialization and reuse them in re-highlighting, then we probably don't need to depose them manually, the vscode [API](https://code.visualstudio.com/api/references/vscode-api#TextEditor.setDecorations) seems do that for us, so I think we'd better to do that in this patch as it would simplify the current implementation. So the APIs would looks like below. In unittest, we just override applyDecorations method. ``` class Highlighter { public Initialize(themeRuleMatcher: ThemeRuleMatcher) { // build a scopeIndex => TextEditorDecorationTypes table } public highlight(fileUri: string, highlightings: SemanticHighlightingLine[]) { // update and patch the highlighting cache. // invoke getDecorations // invoke applyDecorations } protect applyDecorations(decorations...) { // invoke vscode API to apply the decorations. } private getDecorations(hightligtings...) { } ``` ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:176 + // recolorization for all current highlighters. + public updateThemeRuleMatcher(themeRuleMatcher: ThemeRuleMatcher) { + this.themeRuleMatcher = themeRuleMatcher; ---------------- I'd name it `Initialize`. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:182 + // Called when clangd sends an incremental update of highlightings. + public setFileLines(uriString: string, tokens: SemanticHighlightingLine[]) { + // Patch in the new highlightings to the highlightings cache. If this is the ---------------- the name of this public API is confusing, the name indicates it just **sets** the data, but actually it does more stuff like doing the highlightings. I think we may get rid of this function, by just exposing the highlight API. see my above comment. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:199 + // Applies all highlightings to the file with uri uriString. + private highlight(uriString: string) { + if (!this.highlighters.has(uriString)) { ---------------- could we name it fileUri? and elsewhere ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:217 + Array.from(this.files.get(uriString).values()) + .forEach((line) => lines.push(line)); + // Maps scopeIndexes -> the DecorationRangePair used for the scope. ---------------- nit: I think we can just do ` const lines: SemanticHighlightingLine[] = Array.from(this.files.get(uriString).values())`? ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:219 + // Maps scopeIndexes -> the DecorationRangePair used for the scope. + const decorations: Map<number, DecorationRangePair> = new Map(); + lines.forEach((line) => { ---------------- nit: pull out this part into a new method e.g. `getDecorations` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66219/new/ https://reviews.llvm.org/D66219 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits