Repository navigation
Use the clang-tidy version in use for check documentation links - #14830
Abdelhadi Salmaoui (abdelhadi703) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The global cache can select the wrong workspace version, and the fallback is already behind the bundled clang-tidy release.
2 open findings
What changed in this PR
Updates clang-tidy diagnostic links to target documentation matching the selected binary version.
Changes:
- Caches the detected clang-tidy version.
- Centralizes documentation URL generation with fallback handling.
- Adds unit tests and a changelog entry.
| File | Description |
|---|---|
Extension/src/common.ts |
Adds the version cache. |
Extension/src/LanguageServer/settings.ts |
Captures the selected clang-tidy version. |
Extension/src/LanguageServer/codeAnalysis.ts |
Uses version-aware documentation URLs. |
Extension/src/LanguageServer/clangTidyDocumentation.ts |
Builds documentation URLs. |
Extension/test/unit/clangTidyDocumentation.test.ts |
Tests URL generation and fallback behavior. |
Extension/CHANGELOG.md |
Documents the fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| // TODO: This should be checking the clang-tidy version used to better support usage of older versions. | ||
| const primaryDocUri: vscode.Uri = vscode.Uri.parse(`https://releases.llvm.org/23.1.0/tools/clang/tools/extra/docs/clang-tidy/${docPage}`); | ||
| const primaryDocUri: vscode.Uri = vscode.Uri.parse(getClangTidyDocumentationUrl(docPage, new CppSettings().clangTidyVersion)); |
| * Reduce CPU usage when resolving headers in large Windows workspaces. | ||
|
|
||
| ### Bug Fixes | ||
| * Fix clang-tidy check documentation links pointing at the bundled LLVM release instead of the clang-tidy version in use. [#12804](https://github.com/microsoft/vscode-cpptools/issues/12804) |
There was a problem hiding this comment.
We update the changelog ourselves in a separate PR before the release.
Only the .1.0 release of each major has a published documentation page: 23.1.0 and 22.1.0 answer, 23.1.1 and 22.1.3 return 404. The version in use is reduced to its major.1.0 release before it goes into the URL. Also drops the changelog entry, since those are written separately before a release, and stops reporting the cached version for a resource that sets its own codeAnalysis.clangTidy.path -- that cached version comes from a binary the resource is not running.
|
Right on both counts. On the version: I checked, and only the .1.0 release of each major has a page -- 23.1.0 and 22.1.0 return 200, 23.1.1 and 22.1.3 return 404. The version in use is now reduced to its major's .1.0 release before it goes into the URL, so 23.1.1 maps to 23.1.0 and 22.1.3 to 22.1.0. A test pins both, plus the fallback cases. That is the fix the suggestion above would have broken. The changelog entry is gone. One more thing I changed while I was in there: the version was read from a process-global cache, so a folder carrying its own |
| } | ||
| // TODO: This should be checking the clang-tidy version used to better support usage of older versions. | ||
| const primaryDocUri: vscode.Uri = vscode.Uri.parse(`https://releases.llvm.org/23.1.0/tools/clang/tools/extra/docs/clang-tidy/${docPage}`); | ||
| const primaryDocUri: vscode.Uri = vscode.Uri.parse(getClangTidyDocumentationUrl(docPage, new CppSettings(realUri).clangTidyVersion)); |
Every clang-tidy diagnostic carries a link to the documentation of the check, and that link is built from a hardcoded LLVM release (currently 23.1.0). A check that only exists in a newer clang-tidy therefore gets a dead link. #12804 reported exactly this after moving from clang-tidy 18 to 19 and seeing the link stuck on the 18.1.6 page.
settings.ts already works out which clang-tidy will be used -- the bundled one or the one found on PATH -- and compares their versions, but it discarded the version afterwards. I keep it cached next to the clang-tidy path and read it back when the link is built, so the URL points at the release the running binary actually comes from. When the version could not be determined (the user set codeAnalysis.clangTidy.path explicitly, or the binary could not be executed), the link falls back to the bundled release, i.e. the previous behaviour.
The documentation is published per major release rather than per patch: 23.1.0 and 22.1.0 answer, while 23.1.1 and 22.1.3 have no page at all. The version in use is therefore reduced to its major's .1.0 release before it goes into the URL -- otherwise going from 23.1.0 to 23.1.1 would swap a working link for a 404.
The thread also mentions the version-independent documentation URLs, and that is a fair alternative -- it takes the version out of the URL for good. I went with the version in use because that is what the issue title asks for, and because the version-less page always describes the newest release: for someone on an older clang-tidy it links to checks their binary does not have, which is the same kind of mismatch in the other direction. That is a judgement call rather than a certainty, so say the word if you prefer the version-less URL -- it is a much smaller change than this one.
The URL construction now lives in
LanguageServer/clangTidyDocumentation.tsand is covered bytest/unit/clangTidyDocumentation.test.ts: the version in use, a patch release mapping to its major's .1.0, no version, and a malformed version. It runs with the other unit tests throughyarn test. I also ran those cases directly in node, since the extension test harness installs its packages from the private Azure feed and I don't have access to it here.No CHANGELOG entry: those are written separately before a release.
Fixes #12804.