Address post-merge review comments on CommandBuffer (#1033)#1086
Address post-merge review comments on CommandBuffer (#1033)#1086manon-traverse merged 3 commits intollvm:mainfrom
Conversation
| static bool classof(const CommandBuffer *CB) { | ||
| return CB->getKind() == GPUAPI::DirectX; | ||
| } |
There was a problem hiding this comment.
Nit: don't like the name. But I don't really have an alternative. Also, this doesn't seem to be called?
There was a problem hiding this comment.
https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html
A common naming convention is that these enums are “kind”s, to avoid ambiguity with the words “type” or “class” which have overloaded meanings in many contexts within LLVM. Sometimes there will be a natural name for it, like “opcode”. Don’t bikeshed over this; when in doubt use Kind.
Also, this doesn't seem to be called?
Probably for the same reason as #1033 (comment); it'll get used (via the builtin isa<>/dyn_cast<>) when at least Queue::submit() lands, etc.
There was a problem hiding this comment.
Let's do it this way then!
|
@bogner FYI I asked Claude to find/summarize more classes that lack anchors; these are EDIT: And the new |
Define destructors out-of-line for Buffer, CommandBuffer, Fence, ImageComparatorBase, ImageComparatorDiffImage and Texture to avoid vtable and RTTI emission in every translation unit, per LLVM coding standards. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MarijnS95
force-pushed
the
command-buffer-review-fixes
branch
from
f2eb805 to
278efca
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the as<T>() template + BackendAPI static constexpr pattern with classof() methods enabling llvm::isa/cast/dyn_cast. This also fixes the -Wunused-const-variable warning on BackendAPI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MarijnS95
force-pushed
the
command-buffer-review-fixes
branch
from
278efca to
19996c7
Compare
|
CI failure is unrelated to this PR, so I am merging this 👍 |
3 participants
Address post-review comments from @bogner on #1033. These felt significant enough that they weren't worth cramming into an "unrelated" PR but deserve their own clean follow-up in three individual commits.
Buffer,CommandBuffer,Fence,ImageComparatorBase,ImageComparatorDiffImage,Texture) per LLVM coding standardsclassof()) instead of hand-rolledas<T>()+BackendAPI, fixing-Wunused-const-variableAdding
-Werrorto the build is left for a separate PR as it requires a warning audit first — the project inherits compile flags from LLVM's CMake infrastructure and backend SDK headers may introduce additional warnings.