hokein added a comment. thanks, looks better now. Some more comments on the test code.
================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts:3 import * as vscodelc from 'vscode-languageclient'; - +import * as SM from './semantic-highlighting'; /** ---------------- I'd name it `semanticHighlighting` ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:128 + // DecorationTypes for the current theme that are used when highlighting. + private decorationTypes: vscode.TextEditorDecorationType[]; + // The clangd TextMate scope lookup table. ---------------- nit: mention the index is the scopeIndex? ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:135 + // Update the themeRuleMatcher that is used when highlighting. Also triggers a + // recolorization for all current highlighters. Safe to call multiple times. + public initialize(themeRuleMatcher: ThemeRuleMatcher) { ---------------- nit: the comment is stale now. I believe this function is only called whenever we reload a theme or at the first launch of the extension. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:137 + public initialize(themeRuleMatcher: ThemeRuleMatcher) { + this.decorationTypes = this.scopeLookupTable.map((scopes) => { + const options: vscode.DecorationRenderOptions = { ---------------- if this.decorationsTypes is not empty (when this API is called multiple times), I think we need to call the `dispose` method to release the resource they hold. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:139 + const options: vscode.DecorationRenderOptions = { + color : themeRuleMatcher.getBestThemeRule(scopes[0]).foreground, + // If the rangeBehavior is set to Open in any direction the ---------------- just want to confirm: if we fail to find a matched theme rule, we return an empty decoration. I think vscode just doesn't do anything on an empty color? ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:157 + // TextEditor(s). + public highlight(fileUri: string, tokens: SemanticHighlightingLine[]) { + if (!this.files.has(fileUri)) { ---------------- nit: tokens => HighlightingLines. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:190 + protected applyHighlights(fileUri: string) { + if (!this.decorationTypes) + return; ---------------- nit: add a comment, we don't apply highlights when the class is not ready/initialized. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:80 + } + getDecorationRanges(fileUri: string) { + return super.getDecorationRanges(fileUri); ---------------- could you add a comment you override this so that it can be access from the test? ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:93 + // Groups decorations into the scopes used. + let line = [ + {character : 1, length : 2, scopeIndex : 1}, ---------------- nit: please use a more descriptive name, e.g. `HightlightsInLine`. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:94 + let line = [ + {character : 1, length : 2, scopeIndex : 1}, + {character : 5, length : 2, scopeIndex : 1}, ---------------- I believe the test code is correct, but the code here/below is complex and hard to follow. I think we need make the code more readable/understandable. some suggestions: - the line number is implicitly indicated by the index of the array, I think we can add one more field `line` to the structure (call HighlightingTokens). - and creates some helper functions (with proper names) that take the `HighlightingTokens` as parameter and generate the data you need e.g. `SemanticHighlightingLine[]`, `vscode.Range`. 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