Group the node-reuse diagnostics so they can be silenced. - #1881
Draft
vgvassilev wants to merge 1 commit into
Draft
Group the node-reuse diagnostics so they can be silenced.#1881vgvassilev wants to merge 1 commit into
vgvassilev wants to merge 1 commit into
Conversation
The AST-integrity checks added with the no-shared-node work emit a warning a user cannot turn off, and in a release build their findSharedNode/findPrimalSharedNode walks run on every generated derivative even when nobody wants the diagnostic -- pure overhead for a check that only ever fires on a clad bug. Route both diagnostics into the "clad-plugin-node-reuse" group (a subgroup of clad's "clad-plugin" group) through the getCustomDiagID group overload, so -Wno-clad-plugin-node-reuse -- or the wider -Wno-clad-plugin / -Wno-plugin -- silences them. A release build then consults isIgnored and skips the walks entirely when the group is off, so the checks cost nothing once silenced; a debug build always runs them so the asserts still catch a regression. The group overload is a recent clang addition, so a compile-time trait detects it and older clang keeps the ungrouped two-argument form.
Owner
Author
|
Land when/if llvm/llvm-project#208538 lands. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // other warning. Detect the three-argument getCustomDiagID(Level, format, | ||
| // group) overload; older clang has only the two-argument form and the | ||
| // diagnostics stay ungrouped. | ||
| constexpr llvm::StringLiteral CladNodeReuseGroup = "clad-plugin-node-reuse"; |
Contributor
There was a problem hiding this comment.
warning: no header providing "llvm::StringLiteral" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:51:
- #include <memory>
+ #include <llvm/ADT/StringRef.h>
+ #include <memory>| template <typename T> | ||
| struct HasPluginDiagGroups< | ||
| T, std::void_t<decltype(std::declval<T&>().getCustomDiagID( | ||
| clang::DiagnosticsEngine::Warning, std::declval<const char (&)[2]>(), |
Contributor
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
clang::DiagnosticsEngine::Warning, std::declval<const char (&)[2]>(),
^| // group overload. | ||
| template <unsigned N> | ||
| unsigned getIntegrityDiagID(clang::DiagnosticsEngine& Diags, | ||
| const char (&Message)[N]) { |
Contributor
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char (&Message)[N]) {
^| // (or -Wno-clad-plugin, or -Wno-plugin) silences them. | ||
| unsigned SharedDiagID = getIntegrityDiagID( | ||
| Diags, | ||
| "clad internally reused a '%0' AST node while differentiating %1; " |
Contributor
There was a problem hiding this comment.
warning: argument comment missing for literal argument 'Message' [bugprone-argument-comment]
Suggested change
| "clad internally reused a '%0' AST node while differentiating %1; " | |
| /*Message=*/"clad internally reused a '%0' AST node while differentiating %1; " |
| "https://github.com/vgvassilev/clad"); | ||
| unsigned PrimalDiagID = getIntegrityDiagID( | ||
| Diags, | ||
| "clad reused a '%0' AST node from the original function while " |
Contributor
There was a problem hiding this comment.
warning: argument comment missing for literal argument 'Message' [bugprone-argument-comment]
Suggested change
| "clad reused a '%0' AST node from the original function while " | |
| /*Message=*/"clad reused a '%0' AST node from the original function while " |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The AST-integrity checks added with the no-shared-node work emit a warning a user cannot turn off, and in a release build their findSharedNode/findPrimalSharedNode walks run on every generated derivative even when nobody wants the diagnostic -- pure overhead for a check that only ever fires on a clad bug.
Route both diagnostics into the "clad-plugin-node-reuse" group (a subgroup of clad's "clad-plugin" group) through the getCustomDiagID group overload, so -Wno-clad-plugin-node-reuse -- or the wider -Wno-clad-plugin / -Wno-plugin -- silences them. A release build then consults isIgnored and skips the walks entirely when the group is off, so the checks cost nothing once silenced; a debug build always runs them so the asserts still catch a regression.
The group overload is a recent clang addition, so a compile-time trait detects it and older clang keeps the ungrouped two-argument form.