Pipeline Wrapper create #7
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces direct SDL_GPU device/buffer/pipeline usage with wrapper classes (GpuWrapper, BufferWrapper, PipelineWrapper); refactors main and SDL glue to use wrappers for device access, buffer uploads, shader compilation/reload, and pipeline lifecycle; adds shaderc-based compilation, module reorganizations, BufferWrapper upload implementation, and CI/workflow updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as UserApp
participant GW as GpuWrapper
participant BUF as BufferWrapper
participant PW as PipelineWrapper
participant DEV as SDL_GPUDevice
rect rgb(232,243,255)
Note over App,GW: App holds shared_ptr<GpuWrapper>
App->>GW: gpu_wrapper->data()
GW-->>DEV: return SDL_GPUDevice*
end
rect rgb(240,255,232)
Note over App,BUF: Upload vertex data (staging + copy)
App->>BUF: vertex_buffer.upload(data,size,offset)
BUF->>GW: use GW->data() to map/stage and create cmd buffer
GW-->>DEV: submit copy command for GPU transfer
end
rect rgb(255,245,232)
Note over App,PW: Shader set and pipeline rebuild
App->>PW: set_vertex_shader()/set_fragment_shader()
PW->>PW: compile GLSL→SPIRV (shaderc) → mark modified
App->>PW: submit() (per-frame or reload)
PW->>GW: create/recreate graphics pipeline via GW->data()
GW-->>DEV: SDL_CreateGPUGraphicsPipeline(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
28-28: Consider const-correctness and nodiscard attribute.The
data()method should beconstsince it doesn't modify state, and[[nodiscard]]would prevent accidental misuse.- auto data() { return m_graphics_pipeline; } + [[nodiscard]] SDL_GPUGraphicsPipeline* data() const { return m_graphics_pipeline; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
main.cpp(1 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/sdl_wrapper.ixx(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
🔇 Additional comments (4)
sdl_wrapper/CMakeLists.txt (1)
13-13: LGTM!The addition of
sdl_wrapper.pipeline.ixxto the PUBLIC FILES correctly integrates the new pipeline module into the build system.sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
1-7: LGTM!The C++20 module structure and SDL3 header inclusion are correctly implemented.
main.cpp (1)
26-26: Unused member for future implementation.The
pipeline_wrappermember is declared but not used anywhere in the code. This appears to be preparatory work for future pipeline management functionality.Once the issues in
PipelineWrapperare resolved, ensure this member is properly initialized and utilized. Note that the currentPipelineWrapperimplementation has critical initialization issues that need to be addressed before it can be used.sdl_wrapper/sdl_wrapper.ixx (1)
8-8: LGTM!The pipeline submodule is correctly re-exported, following the same pattern as the existing buffer and app submodules. This properly exposes
PipelineWrapperthrough the mainsdl_wrappermodule interface.
sdl_wrapper/sdl_wrapper.pipeline.ixx
Outdated
| export class PipelineWrapper | ||
| { | ||
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | ||
| SDL_GPUDevice* m_device{}; | ||
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | ||
| bool modified = false; | ||
|
|
||
| public: | ||
| PipelineWrapper() = default; | ||
| ~PipelineWrapper() | ||
| { | ||
| if (m_graphics_pipeline) | ||
| { | ||
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | ||
| m_graphics_pipeline = nullptr; | ||
| } | ||
| } | ||
|
|
||
| auto data() { return m_graphics_pipeline; } | ||
|
|
||
| auto submit() | ||
| { | ||
| if (modified) | ||
| { | ||
| modified = false; | ||
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | ||
| m_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Major: Missing copy/move semantics for RAII wrapper.
PipelineWrapper manages SDL resources via raw pointers but doesn't define copy/move semantics. This allows implicit copying, which would lead to double-free when both copies are destroyed.
Add proper resource management:
export class PipelineWrapper
{
SDL_GPUGraphicsPipeline* m_graphics_pipeline{};
SDL_GPUDevice* m_device{};
SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{};
bool modified = false;
public:
PipelineWrapper() = default;
+
+ // Delete copy operations to prevent double-free
+ PipelineWrapper(const PipelineWrapper&) = delete;
+ PipelineWrapper& operator=(const PipelineWrapper&) = delete;
+
+ // Implement move operations
+ PipelineWrapper(PipelineWrapper&& other) noexcept
+ : m_graphics_pipeline(other.m_graphics_pipeline)
+ , m_device(other.m_device)
+ , m_pipeline_info(other.m_pipeline_info)
+ , modified(other.modified)
+ {
+ other.m_graphics_pipeline = nullptr;
+ other.m_device = nullptr;
+ }
+
+ PipelineWrapper& operator=(PipelineWrapper&& other) noexcept
+ {
+ if (this != &other)
+ {
+ // Release current resources
+ if (m_graphics_pipeline && m_device)
+ {
+ SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline);
+ }
+
+ // Transfer ownership
+ m_graphics_pipeline = other.m_graphics_pipeline;
+ m_device = other.m_device;
+ m_pipeline_info = other.m_pipeline_info;
+ modified = other.modified;
+
+ other.m_graphics_pipeline = nullptr;
+ other.m_device = nullptr;
+ }
+ return *this;
+ }
+
~PipelineWrapper()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| ~PipelineWrapper() | |
| { | |
| if (m_graphics_pipeline) | |
| { | |
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | |
| m_graphics_pipeline = nullptr; | |
| } | |
| } | |
| auto data() { return m_graphics_pipeline; } | |
| auto submit() | |
| { | |
| if (modified) | |
| { | |
| modified = false; | |
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | |
| m_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); | |
| } | |
| } | |
| }; | |
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| // Delete copy operations to prevent double-free | |
| PipelineWrapper(const PipelineWrapper&) = delete; | |
| PipelineWrapper& operator=(const PipelineWrapper&) = delete; | |
| // Implement move operations | |
| PipelineWrapper(PipelineWrapper&& other) noexcept | |
| : m_graphics_pipeline(other.m_graphics_pipeline) | |
| , m_device(other.m_device) | |
| , m_pipeline_info(other.m_pipeline_info) | |
| , modified(other.modified) | |
| { | |
| other.m_graphics_pipeline = nullptr; | |
| other.m_device = nullptr; | |
| } | |
| PipelineWrapper& operator=(PipelineWrapper&& other) noexcept | |
| { | |
| if (this != &other) | |
| { | |
| // Release current resources | |
| if (m_graphics_pipeline && m_device) | |
| { | |
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | |
| } | |
| // Transfer ownership | |
| m_graphics_pipeline = other.m_graphics_pipeline; | |
| m_device = other.m_device; | |
| m_pipeline_info = other.m_pipeline_info; | |
| modified = other.modified; | |
| other.m_graphics_pipeline = nullptr; | |
| other.m_device = nullptr; | |
| } | |
| return *this; | |
| } | |
| ~PipelineWrapper() | |
| { | |
| if (m_graphics_pipeline) | |
| { | |
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | |
| m_graphics_pipeline = nullptr; | |
| } | |
| } | |
| auto data() { return m_graphics_pipeline; } | |
| auto submit() | |
| { | |
| if (modified) | |
| { | |
| modified = false; | |
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | |
| m_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.ixx around lines 10 to 39, PipelineWrapper
is an RAII wrapper that manages raw SDL pointers but lacks copy/move semantics
so implicit copies will cause double-free; fix by explicitly deleting copy
constructor and copy assignment to prevent copying, and implement move
constructor and move assignment that transfer ownership (move device, pipeline
pointer, pipeline_info and modified flag) and set the source's pointers/flags to
null/false; ensure the destructor and submit use the moved-to state safely
(check for null before release) and that submit recreates the pipeline only when
device is valid, so moved-from objects are left in a safe, no-op state.
sdl_wrapper/sdl_wrapper.pipeline.ixx
Outdated
| export class PipelineWrapper | ||
| { | ||
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | ||
| SDL_GPUDevice* m_device{}; | ||
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | ||
| bool modified = false; | ||
|
|
||
| public: | ||
| PipelineWrapper() = default; |
There was a problem hiding this comment.
Critical: Class lacks initialization mechanism.
The PipelineWrapper class has no way to initialize m_device or m_pipeline_info, making it unusable. The defaulted constructor leaves these members uninitialized, and there are no setter methods or parameterized constructors.
Consider adding a parameterized constructor:
- PipelineWrapper() = default;
+ PipelineWrapper() = default;
+ PipelineWrapper(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info)
+ : m_device(device), m_pipeline_info(info), modified(true) {}Or add initialization methods:
+ void initialize(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info)
+ {
+ m_device = device;
+ m_pipeline_info = info;
+ modified = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| PipelineWrapper(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info) | |
| : m_device(device), m_pipeline_info(info), modified(true) {} |
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| export class PipelineWrapper | |
| { | |
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | |
| SDL_GPUDevice* m_device{}; | |
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | |
| bool modified = false; | |
| public: | |
| PipelineWrapper() = default; | |
| void initialize(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info) | |
| { | |
| m_device = device; | |
| m_pipeline_info = info; | |
| modified = true; | |
| } |
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.ixx around lines 10 to 18, the
PipelineWrapper class currently only has a default constructor leaving m_device
and m_pipeline_info effectively uninitialized; add a clear initialization
mechanism by implementing either a parameterized constructor that accepts an
SDL_GPUDevice* and an SDL_GPUGraphicsPipelineCreateInfo (or const ref) and
initializes m_device, m_pipeline_info and modified (false), or provide explicit
init/set methods (e.g., initialize(device, pipelineInfo) and/or setters for
individual members) that perform the same assignment and validate non-null
device values; also ensure any ownership/cleanup expectations are documented or
handled (raw pointer vs. owning) and set modified appropriately after
initialization or subsequent setters.
sdl_wrapper/sdl_wrapper.pipeline.ixx
Outdated
| SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; | ||
| SDL_GPUDevice* m_device{}; | ||
| SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; | ||
| bool modified = false; |
There was a problem hiding this comment.
Design issue: No way to mark pipeline as modified.
The modified flag is checked in submit() but there's no public method to set it to true. This makes the conditional rebuild logic in submit() unreachable.
Consider adding methods to update the pipeline configuration:
+ void update_pipeline_info(const SDL_GPUGraphicsPipelineCreateInfo& info)
+ {
+ m_pipeline_info = info;
+ modified = true;
+ }
+
+ void mark_modified()
+ {
+ modified = true;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.ixx around line 15, the private bool
modified flag is never exposed or updated, making the conditional rebuild in
submit() unreachable; add a minimal public API to update this flag (e.g., a
markModified() method and/or setModified(bool)) and call it from any
pipeline-configuration mutators (or document callers must call it after changes)
so submit() can detect changes and trigger rebuilds.
There was a problem hiding this comment.
@WSQS, understood! Thanks for the update. Feel free to reach out if you'd like me to review the implementation once it's ready. 🐰
sdl_wrapper/sdl_wrapper.pipeline.ixx
Outdated
| ~PipelineWrapper() | ||
| { | ||
| if (m_graphics_pipeline) | ||
| { | ||
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | ||
| m_graphics_pipeline = nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify SDL behavior with nullptr device parameter.
The destructor calls SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline) without checking if m_device is nullptr. If the pipeline was somehow created but the device pointer is invalid, this could cause issues.
Consider adding a device check:
~PipelineWrapper()
{
- if (m_graphics_pipeline)
+ if (m_graphics_pipeline && m_device)
{
SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline);
m_graphics_pipeline = nullptr;
}
}🌐 Web query:
Does SDL_ReleaseGPUGraphicsPipeline handle nullptr device parameter safely?
💡 Result:
Short answer: No guarantee — the docs don't state that SDL_ReleaseGPUGraphicsPipeline accepts a NULL device, so you should not rely on it handling a nullptr safely. Use the same SDL_GPUDevice you used to create the pipeline (or obtain it via SDL_GetGPURendererDevice) when calling SDL_ReleaseGPUGraphicsPipeline. [1][2]
Sources:
- SDL wiki: SDL_ReleaseGPUGraphicsPipeline (signature shows SDL_GPUDevice *device, no note that NULL is accepted). [1]
- libsdl manpage for SDL_ReleaseGPUGraphicsPipeline (same signature and remarks). [2]
Add device validity check before releasing GPU pipeline.
The destructor must verify m_device is non-null before calling SDL_ReleaseGPUGraphicsPipeline. Per SDL documentation, the function does not safely handle a nullptr device parameter. Apply the suggested fix:
~PipelineWrapper()
{
- if (m_graphics_pipeline)
+ if (m_graphics_pipeline && m_device)
{
SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline);
m_graphics_pipeline = nullptr;
}
}🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.ixx around lines 19 to 26, the destructor
calls SDL_ReleaseGPUGraphicsPipeline without verifying m_device is valid; update
the destructor to check that m_device is non-null (and m_graphics_pipeline as
already checked) before calling SDL_ReleaseGPUGraphicsPipeline, then release and
set m_graphics_pipeline to nullptr as before.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sdl_wrapper/sdl_wrapper.pipeline.ixx (2)
10-18: Critical: Class design has multiple blocking issues.Three critical problems remain unresolved from previous reviews:
- Missing copy/move semantics - Implicit copying causes double-free. The class manages raw SDL pointers without defining copy/move operations.
- No initialization mechanism -
m_deviceandm_pipeline_infocannot be set, making the class unusable.- No API to mark modified - The
modifiedflag starts atfalsewith no public method to set ittrue, sosubmit()never rebuilds.Apply the fixes suggested in previous reviews:
- Delete copy operations and implement move semantics (see previous review comment on lines 10-42)
- Add a parameterized constructor or initialization method (see previous review comment on lines 10-18)
- Add a
mark_modified()orupdate_pipeline_info()method (see previous review comment on line 15)
19-26: Critical: Add device validity check before releasing pipeline.The destructor must verify
m_deviceis non-null before callingSDL_ReleaseGPUGraphicsPipeline. SDL documentation confirms the function does not safely handle a nullptr device parameter.Apply this fix:
~PipelineWrapper() { - if (m_graphics_pipeline) + if (m_graphics_pipeline && m_device) { SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); m_graphics_pipeline = nullptr; } }
🧹 Nitpick comments (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
28-28: Consider adding const-correctness.The
data()method could provide const-qualified overloads for read-only access patterns.auto data() { return m_graphics_pipeline; } auto data() const { return m_graphics_pipeline; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
🔇 Additional comments (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
30-41: Good fix on pipeline null check; add device validation and error handling.The null check on
m_graphics_pipelinebefore release (lines 35-38) addresses the previous critical issue. However, two concerns remain:
- Missing device validity check - For consistency with the destructor fix, verify
m_deviceis non-null before both releasing and creating pipelines.- No error handling -
SDL_CreateGPUGraphicsPipelinecan return nullptr on failure, leavingm_graphics_pipelinein a null state with no indication to the caller.Consider this improvement:
auto submit() { if (modified) { modified = false; - if (m_graphics_pipeline) + if (m_graphics_pipeline && m_device) { SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); } - m_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); + if (m_device) + { + m_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); + // Consider: check if m_graphics_pipeline is null and handle error + } } }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
sdl_wrapper/sdl_wrapper.pipeline.ixx (4)
11-19: Critical: Missing copy/move semantics causes double-free.The class manages raw SDL pointers but doesn't define copy/move operations. Implicit copying would cause double-free when both copies are destroyed.
Apply this diff to prevent unsafe copying and enable move semantics:
export class PipelineWrapper { SDL_GPUGraphicsPipeline* m_graphics_pipeline{}; SDL_GPUDevice* m_device{}; SDL_GPUGraphicsPipelineCreateInfo m_pipeline_info{}; bool modified = false; public: PipelineWrapper() = default; + + // Delete copy operations to prevent double-free + PipelineWrapper(const PipelineWrapper&) = delete; + PipelineWrapper& operator=(const PipelineWrapper&) = delete; + + // Implement move operations + PipelineWrapper(PipelineWrapper&& other) noexcept + : m_graphics_pipeline(std::exchange(other.m_graphics_pipeline, nullptr)) + , m_device(std::exchange(other.m_device, nullptr)) + , m_pipeline_info(other.m_pipeline_info) + , modified(other.modified) + { + } + + PipelineWrapper& operator=(PipelineWrapper&& other) noexcept + { + if (this != &other) + { + // Release current resources + if (m_graphics_pipeline && m_device) + { + SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); + } + + // Transfer ownership + m_graphics_pipeline = std::exchange(other.m_graphics_pipeline, nullptr); + m_device = std::exchange(other.m_device, nullptr); + m_pipeline_info = other.m_pipeline_info; + modified = other.modified; + } + return *this; + }
11-19: Critical: No initialization mechanism.The class has no way to initialize
m_deviceorm_pipeline_info. The default constructor leaves these uninitialized, making the class unusable.Add a parameterized constructor:
public: PipelineWrapper() = default; + PipelineWrapper(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info) + : m_device(device), m_pipeline_info(info), modified(true) + { + }Or provide initialization methods:
+ void initialize(SDL_GPUDevice* device, const SDL_GPUGraphicsPipelineCreateInfo& info) + { + m_device = device; + m_pipeline_info = info; + modified = true; + }
20-27: Critical: Missing device validity check in destructor.The destructor calls
SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline)without verifying thatm_deviceis non-null. SDL documentation does not guarantee safe handling of nullptr device parameters.Apply this fix:
~PipelineWrapper() { - if (m_graphics_pipeline) + if (m_graphics_pipeline && m_device) { SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); m_graphics_pipeline = nullptr; } }
16-16: Minor: Modified flag cannot be set to true.The
modifiedflag is checked insubmit()but there's no public method to set it totrue, making the rebuild logic unreachable.Add methods to update the pipeline configuration:
+ void update_pipeline_info(const SDL_GPUGraphicsPipelineCreateInfo& info) + { + m_pipeline_info = info; + modified = true; + } + + void mark_modified() + { + modified = true; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, clang)
sdl_wrapper/sdl_wrapper.pipeline.ixx
Outdated
| auto submit() | ||
| { | ||
| if (modified) | ||
| { | ||
| modified = false; | ||
| auto new_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info); | ||
| if (new_graphics_pipeline) { | ||
| if (m_graphics_pipeline) { | ||
| SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline); | ||
| } | ||
| m_graphics_pipeline = new_graphics_pipeline; | ||
| } else { | ||
| SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s get error:%s",__FUNCTION__, SDL_GetError()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Missing device validation in submit().
The method uses m_device on lines 36 and 39 without checking if it's non-null. If m_device is nullptr (default state), this will cause undefined behavior or crashes.
Add device validation:
auto submit()
{
+ if (!m_device)
+ {
+ SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s: device is null", __FUNCTION__);
+ return;
+ }
+
if (modified)
{
modified = false;
auto new_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device, &m_pipeline_info);
if (new_graphics_pipeline) {
if (m_graphics_pipeline) {
SDL_ReleaseGPUGraphicsPipeline(m_device, m_graphics_pipeline);
}
m_graphics_pipeline = new_graphics_pipeline;
} else {
SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s get error:%s",__FUNCTION__, SDL_GetError());
}
}
}🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.ixx around lines 31 to 46, submit() uses
m_device when calling SDL_CreateGPUGraphicsPipeline and
SDL_ReleaseGPUGraphicsPipeline without validating it; add an early guard that
checks if m_device is nullptr and, if so, log an error and return immediately to
avoid undefined behavior. Ensure subsequent calls to
SDL_CreateGPUGraphicsPipeline and SDL_ReleaseGPUGraphicsPipeline only run when
m_device is non-null, preserving the existing modified flag behavior and
resetting modified=false only when appropriate.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
main.cpp (1)
26-30: Consider usingPipelineWrapperto managegraphicsPipeline.You've introduced
pipeline_wrapper(line 26) but never used it, whilegraphicsPipeline(line 30) continues to be managed manually with raw SDL calls. This creates inconsistency—the GPU device is wrapped, but the pipeline is not.Based on the PR objectives mentioning
PipelineWrapper, consider completing the abstraction by managinggraphicsPipelinethroughpipeline_wrapper. This would provide consistent resource management and align with the wrapper pattern used for the GPU device.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
main.cpp(9 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/sdl_callback_implement.cpp(1 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)sdl_wrapper/sdl_wrapper.ixx(1 hunks)
🔇 Additional comments (2)
sdl_wrapper/CMakeLists.txt (1)
13-14: LGTM!The new module files are correctly added to the public module surface.
sdl_wrapper/sdl_wrapper.ixx (1)
8-9: LGTM!The new module exports are correctly declared and consistent with the existing module structure.
| extern sopho::App *create_app(); | ||
|
|
||
| SDL_AppResult SDL_AppInit(void **appstate, int argc, char **argv) { | ||
| SDL_Init(SDL_INIT_VIDEO); |
There was a problem hiding this comment.
Check SDL_Init return value.
SDL_Init can fail and returns a non-zero value on failure. If initialization fails, all subsequent SDL operations will fail or crash. You must check the return value and handle errors appropriately.
Apply this diff to add error handling:
- SDL_Init(SDL_INIT_VIDEO);
+ if (SDL_Init(SDL_INIT_VIDEO) != 0) {
+ SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to initialize SDL: %s", SDL_GetError());
+ return SDL_APP_FAILURE;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SDL_Init(SDL_INIT_VIDEO); | |
| if (SDL_Init(SDL_INIT_VIDEO) != 0) { | |
| SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to initialize SDL: %s", SDL_GetError()); | |
| return SDL_APP_FAILURE; | |
| } |
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_callback_implement.cpp around line 12,
SDL_Init(SDL_INIT_VIDEO) is called without checking its return value; change
this to check if SDL_Init(...) != 0 and, on failure, retrieve the error with
SDL_GetError() and handle it (e.g., log or print the error, perform any
necessary cleanup such as SDL_Quit(), and return an error code or call
exit(EXIT_FAILURE) so subsequent SDL calls are not executed).
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_callback_implement.cpp`
* 📝 Add docstrings to `pipiline` Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_callback_implement.cpp` * style: format code with ClangFormat This commit fixes the style issues introduced in 49e577b according to the output from ClangFormat. Details: #8 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdl_wrapper/sdl_wrapper.buffer.ixx (1)
63-72: Reorder member variables to fix use-after-free in destructor.The TODO comment at line 64 correctly identifies a critical issue:
gpu_wrapperis destroyed beforevertexBufferdue to C++ member destruction order (reverse declaration order). WhenBufferWrapper::~BufferWrapper()runs, it callsSDL_ReleaseGPUBuffer(m_gpu->data(), ...), butm_gpu->data()returns a pointer to an already-destroyed GPU device (destroyed inGpuWrapper::~GpuWrapper()).Fix: Declare
gpu_wrapperbeforevertexBufferin theUserAppclass (main.cpp lines 25–27) so it is destroyed last. This ensures all GPU buffers are released before the GPU device is destroyed.class UserApp : public sopho::App { + std::shared_ptr<sopho::GpuWrapper> gpu_wrapper{std::make_shared<sopho::GpuWrapper>()}; std::optional<sopho::BufferWrapper> vertexBuffer; std::optional<sopho::PipelineWrapper> pipeline_wrapper; - std::shared_ptr<sopho::GpuWrapper> gpu_wrapper{std::make_shared<sopho::GpuWrapper>()};
♻️ Duplicate comments (2)
main.cpp (2)
26-26: Remove unusedpipeline_wrappermember.The
pipeline_wrappermember is declared but never used in the code. The graphics pipeline is still managed directly viagraphicsPipeline(line 30) instead of through the wrapper.Apply this diff to remove the unused member:
- std::optional<sopho::PipelineWrapper> pipeline_wrapper; std::shared_ptr<sopho::GpuWrapper> gpu_wrapper{std::make_shared<sopho::GpuWrapper>()};Alternatively, if you intend to use
PipelineWrapperto managegraphicsPipeline, complete the implementation by initializing and routing pipeline operations through the wrapper.
86-86: Add null checks forgpu_wrapper->data()calls.Throughout
init(),gpu_wrapper->data()is called without verifying that the device pointer is valid. IfGpuWrapperconstruction fails, all these calls will passnullptrto SDL functions, causing undefined behavior or crashes.Consider one of these approaches:
Fix GpuWrapper to throw on construction failure, ensuring the device is always valid.
Add validation in
init()after creatinggpu_wrapper:+ if (!gpu_wrapper || !gpu_wrapper->data()) { + SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to create GPU device"); + return SDL_APP_FAILURE; + } + SDL_ClaimWindowForGPUDevice(gpu_wrapper->data(), window);Also applies to: 112-112, 137-137, 178-178, 184-184, 190-190, 219-220
🧹 Nitpick comments (1)
sdl_wrapper/sdl_wrapper.buffer.ixx (1)
26-57: The refactor is correct, but note the TODO for batching commands.All GPU API calls correctly route through
m_gpu->data(). However, the TODO at line 40 flags a performance concern: eachupload()call immediately submits a command buffer, which can be inefficient if multiple uploads occur per frame.Do you want me to generate a design proposal for batching upload commands, or open an issue to track this optimization?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main.cpp(10 hunks)sdl_wrapper/sdl_wrapper.buffer.ixx(2 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl_wrapper/sdl_wrapper.gpu.ixx
🔇 Additional comments (6)
sdl_wrapper/sdl_wrapper.buffer.ixx (2)
5-9: LGTM!The addition of
<memory>forstd::shared_ptrand the import of:gpumodule are necessary and correctly support the refactor to useGpuWrapper.
13-13: LGTM!The refactor from raw
SDL_GPUDevice*tostd::shared_ptr<GpuWrapper>improves lifetime management and enables safe sharing of the GPU wrapper across multiple buffer instances.main.cpp (4)
73-80: LGTM!The docstrings for
init(),iterate(), andquit()are well-formatted, clear, and improve code documentation.
265-299: LGTM! Live shader recompilation is well-implemented.The live shader editor correctly recompiles shaders, releases old resources, and recreates the graphics pipeline on the fly. The error handling logs compilation errors appropriately.
378-393: Verify member destruction order for safe cleanup.In
quit(), GPU resources are released manually, butvertexBuffer(which also holds GPU resources) relies on its destructor. Since C++ destroys members in reverse declaration order,vertexBuffer(line 25) will be destroyed beforegpu_wrapper(line 27), which is correct. However, the explicit cleanup here for shaders and pipeline should ideally match.Consider explicitly resetting
vertexBufferinquit()for consistency:// release the pipeline SDL_ReleaseGPUGraphicsPipeline(gpu_wrapper->data(), graphicsPipeline); + + // release the vertex buffer + vertexBuffer.reset(); // destroy the window SDL_DestroyWindow(window);This makes the cleanup order explicit and matches the pattern used for other resources.
27-27: No initialization order issue found—SDL_Init is correctly called before GpuWrapper construction.Verification shows the initialization order is correct:
- SDL framework calls
SDL_AppInit()(sdl_wrapper/sdl_callback_implement.cpp:19)SDL_Init(SDL_INIT_VIDEO)is called at line 21create_app()is called at line 22, which constructsUserAppGpuWrapper's constructor runs during member initialization and callsSDL_CreateGPUDevice()with SDL already initializedThe original concern is resolved.
sdl_wrapper/sdl_wrapper.buffer.ixx
Outdated
| BufferWrapper(std::shared_ptr<GpuWrapper> p_gpu, const SDL_GPUBufferCreateInfo* p_create_info) : | ||
| m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(m_gpu->data(), p_create_info)) | ||
| { | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that GpuWrapper never returns null, or add defensive checks.
The constructor passes m_gpu->data() directly to SDL_CreateGPUBuffer without checking whether the device pointer is valid. If GpuWrapper::data() returns nullptr (e.g., if GPU device creation failed), m_vertex_buffer will be nullptr and subsequent operations will crash or exhibit undefined behavior.
Consider one of these approaches:
- Ensure
GpuWrapperthrows on construction failure so thatdata()is guaranteed non-null. - Add a check in the constructor:
BufferWrapper(std::shared_ptr<GpuWrapper> p_gpu, const SDL_GPUBufferCreateInfo* p_create_info) :
-m_gpu(p_gpu), m_vertex_buffer(SDL_CreateGPUBuffer(m_gpu->data(), p_create_info))
+m_gpu(p_gpu)
{
+ if (!m_gpu || !m_gpu->data()) {
+ throw std::runtime_error("Invalid GPU device in BufferWrapper constructor");
+ }
+ m_vertex_buffer = SDL_CreateGPUBuffer(m_gpu->data(), p_create_info);
+ if (!m_vertex_buffer) {
+ throw std::runtime_error("Failed to create GPU buffer");
+ }
}Run the following script to check if GpuWrapper provides null-safety guarantees:
🏁 Script executed:
#!/bin/bash
# Description: Check GpuWrapper implementation for null-safety guarantees.
# Search for GpuWrapper class definition and data() method
ast-grep --pattern $'class GpuWrapper {
$$$
}'
# Check for validation in GpuWrapper constructor
rg -nP --type=cpp -A10 'class\s+GpuWrapper'
# Look for data() method implementation
rg -nP --type=cpp -A5 'data\(\)\s*(const)?\s*\{'Length of output: 0
I need to search more broadly to locate the GpuWrapper definition. Let me run additional verification scripts:
Wait, I should not skip cloning since I need to search the codebase. Let me fix that:
🏁 Script executed:
#!/bin/bash
# Broader search for GpuWrapper definition - check all file types
rg -i "GpuWrapper" --type-list | head -20
# Search for GpuWrapper in all file types
rg "GpuWrapper" -l
# Search for class GpuWrapper without strict pattern
rg "class\s+GpuWrapper" -n
# Also check for struct GpuWrapper
rg "struct\s+GpuWrapper" -n
# List relevant header/source files
fd -e hpp -e h -e ixx -e cpp | grep -i gpu | head -20Length of output: 562
🏁 Script executed:
#!/bin/bash
# Broader search for GpuWrapper definition
rg "GpuWrapper" -l
# Search for class or struct GpuWrapper
rg "(class|struct)\s+GpuWrapper" -n -C5
# Find all GPU-related header files
fd -e hpp -e h -e ixx | grep -i gpu
# Check file structure
fd -e ixx | head -20Length of output: 965
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full GpuWrapper constructor implementation
rg -n "GpuWrapper\(\)" sdl_wrapper/sdl_wrapper.gpu.ixx -A20
# Search for data() method in GpuWrapper
rg -n "data\(\)" sdl_wrapper/sdl_wrapper.gpu.ixx -B2 -A5
# Get the full GpuWrapper class
rg -n "class GpuWrapper" sdl_wrapper/sdl_wrapper.gpu.ixx -A50
Length of output: 1301
---
🏁 Script executed:
```shell
#!/bin/bash
# Check BufferWrapper class and how m_vertex_buffer is used
rg -n "class BufferWrapper" sdl_wrapper/sdl_wrapper.buffer.ixx -A100
# Search for uses of m_vertex_buffer in buffer file
rg -n "m_vertex_buffer" sdl_wrapper/sdl_wrapper.buffer.ixx -B2 -A2
Length of output: 3713
Add defensive null checks in BufferWrapper constructor and methods, or require GpuWrapper to throw on failure.
Verification confirms the issue: GpuWrapper::data() can return nullptr (if SDL_CreateGPUDevice fails, the constructor only logs the error without throwing), and SDL_CreateGPUBuffer can also fail and return nullptr. The constructor passes these potentially-null pointers without validation, and m_vertex_buffer is later used unsafely in upload() (lines 49, 53) and the destructor (line 65) without null checks.
The suggested fix approach is sound. Either:
- Modify
GpuWrapperconstructor to throw onSDL_CreateGPUDevicefailure (not just log), or - Add guards in
BufferWrapperconstructor to validatem_gpu->data()and check the result ofSDL_CreateGPUBuffer, throwing if either is null.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_wrapper.buffer.cpp` * `sdl_wrapper/sdl_wrapper.pipeline.cpp`
* 📝 Add docstrings to `pipiline` Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_wrapper.buffer.cpp` * `sdl_wrapper/sdl_wrapper.pipeline.cpp` * style: format code with ClangFormat This commit fixes the style issues introduced in c7383b5 according to the output from ClangFormat. Details: #17 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.cpp (1)
306-352: CheckSDL_AcquireGPUCommandBufferbefore use
SDL_AcquireGPUCommandBuffercan return NULL; we pass the result straight intoSDL_WaitAndAcquireGPUSwapchainTextureand later submit it, so a transient allocation failure turns into an immediate crash. Add a guard (log + early return/cancel) when acquisition fails. (manpages.debian.org)
♻️ Duplicate comments (1)
sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
11-26: Make PipelineWrapper move-only before storing in std::optional
PipelineWrapperstill has the compiler-generated copy/move. Returning it by value fromGpuWrapper::create_pipeline()and then materialising it insidestd::optionalcopies the rawSDL_GPUGraphicsPipeline*and shared device pointer; the temporary’s destructor immediately callsSDL_ReleaseGPUGraphicsPipelinewhile the optional retains the now-invalid pointer, setting us up for double-release or use-after-free. Please delete the copy constructor/assignment and implement move operations that transfer ownership and null out the source before destroying it. Also gateSDL_ReleaseGPUGraphicsPipelinebehind a valid device pointer. (manpages.debian.org)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gen-pr.config.yml(1 hunks)main.cpp(14 hunks)sdl_wrapper/sdl_wrapper.buffer.cpp(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- gen-pr.config.yml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cppsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.ixxsdl_wrapper/sdl_wrapper.pipeline.cpp
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cppsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.ixxsdl_wrapper/sdl_wrapper.pipeline.cpp
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
main.cppsdl_wrapper/sdl_wrapper.buffer.cppsdl_wrapper/sdl_wrapper.pipeline.cpp
| void BufferWrapper::upload(void* p_data, uint32_t p_size, uint32_t p_offset) | ||
| { | ||
| if (p_size > m_transfer_buffer_size) | ||
| { | ||
| if (m_transfer_buffer != nullptr) | ||
| { | ||
| SDL_ReleaseGPUTransferBuffer(m_gpu->data(), m_transfer_buffer); | ||
| } | ||
| SDL_GPUTransferBufferCreateInfo transfer_info{SDL_GPU_TRANSFERBUFFERUSAGE_UPLOAD, p_size, 0}; | ||
| m_transfer_buffer = SDL_CreateGPUTransferBuffer(m_gpu->data(), &transfer_info); | ||
| m_transfer_buffer_size = transfer_info.size; | ||
| } | ||
|
|
||
| auto data = SDL_MapGPUTransferBuffer(m_gpu->data(), m_transfer_buffer, false); | ||
| SDL_memcpy(data, p_data, p_size); | ||
| SDL_UnmapGPUTransferBuffer(m_gpu->data(), m_transfer_buffer); | ||
|
|
||
| // TODO: Delay submit command in collect | ||
| auto command_buffer = SDL_AcquireGPUCommandBuffer(m_gpu->data()); | ||
| auto copy_pass = SDL_BeginGPUCopyPass(command_buffer); | ||
|
|
||
| SDL_GPUTransferBufferLocation location{}; | ||
| location.transfer_buffer = m_transfer_buffer; | ||
| location.offset = 0; | ||
|
|
||
| SDL_GPUBufferRegion region{}; | ||
| region.buffer = m_vertex_buffer; | ||
| region.size = p_size; | ||
| region.offset = p_offset; | ||
|
|
||
| SDL_UploadToGPUBuffer(copy_pass, &location, ®ion, false); | ||
|
|
||
| SDL_EndGPUCopyPass(copy_pass); | ||
| SDL_SubmitGPUCommandBuffer(command_buffer); | ||
| } |
There was a problem hiding this comment.
Guard GPU upload path against NULL returns
SDL_CreateGPUTransferBuffer, SDL_MapGPUTransferBuffer, and SDL_AcquireGPUCommandBuffer all document NULL on failure. We free the old staging buffer, record the requested size, and then continue even if creation/mapping fails, so a transient allocation error leaves m_transfer_buffer_size non-zero, m_transfer_buffer still NULL, and the subsequent SDL_memcpy, copy pass setup, and command submission go through null pointers. That's an immediate crash risk and permanently bricks future uploads for the same size. Please bail out (and reset size) whenever any of these steps return NULL, and don’t proceed to map/copy/submit without a valid buffer/command buffer. (manpages.debian.org)
🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.buffer.cpp around lines 48 to 82: the code assumes
SDL_CreateGPUTransferBuffer, SDL_MapGPUTransferBuffer and
SDL_AcquireGPUCommandBuffer never return NULL; if any do you must bail and reset
state to avoid crashes and permanently bricking uploads. Change the flow so that
you only update m_transfer_buffer and m_transfer_buffer_size after a successful
SDL_CreateGPUTransferBuffer (if creation returns NULL do not update size, keep
m_transfer_buffer nullptr and return early), check the result of
SDL_MapGPUTransferBuffer and if it returns NULL unmap/free/leave state
consistent and return early, and check SDL_AcquireGPUCommandBuffer for NULL
before beginning a copy pass; in any early-return path restore/leave
m_transfer_buffer_size as 0 and do not call SDL_memcpy, SDL_UploadToGPUBuffer,
SDL_EndGPUCopyPass or SDL_SubmitGPUCommandBuffer when required resources are
NULL.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main.cpp (2)
86-226: Shader compilation errors are logged but not handled;init()continues on failure.Lines 96-100 and 121-125 detect shader compilation failures and log errors, but
init()continues execution and returnsSDL_APP_CONTINUEeven when shader creation fails. This will result in null shader pointers being passed toSDL_CreateGPUGraphicsPipelineon Line 189, causing undefined behavior.Consider one of these approaches:
Option 1 (recommended): Return early on shader compilation failure:
if (result.GetCompilationStatus() != shaderc_compilation_status_success) { SDL_LogError(SDL_LOG_CATEGORY_RENDER, "[shaderc] compile error in test.glsl: %s", result.GetErrorMessage().data()); + return SDL_APP_FAILURE; }Apply the same pattern at Line 121-125 for fragment shader compilation.
Option 2: Validate shader pointers before pipeline creation:
if (vertexShader == nullptr || fragmentShader == nullptr) { SDL_LogError(SDL_LOG_CATEGORY_RENDER, "Shader creation failed"); return SDL_APP_FAILURE; }
263-298: Inconsistent error logging and unsafe shader/pipeline recreation pattern.Line 270 uses
std::cerrfor shader compilation errors, whileinit()usesSDL_LogError(Lines 98, 123). Additionally, Lines 291-292 release the old shader before validating the new one—ifSDL_CreateGPUShaderfails and returns null,vertexShaderbecomes null and will cause crashes on the next frame when bound to the pipeline.Apply this diff to use consistent logging and validate before replacing:
if (result.GetCompilationStatus() != shaderc_compilation_status_success) { - std::cerr << "[shaderc] compile error in " << "test.glsl" << ":\n" - << result.GetErrorMessage() << std::endl; + SDL_LogError(SDL_LOG_CATEGORY_RENDER, "[shaderc] compile error in test.glsl: %s", + result.GetErrorMessage().data()); } else { // load the vertex shader code std::vector<uint32_t> vertexCode{result.cbegin(), result.cend()}; // create the vertex shader SDL_GPUShaderCreateInfo vertexInfo{}; vertexInfo.code = (Uint8*)vertexCode.data(); vertexInfo.code_size = vertexCode.size() * 4; vertexInfo.entrypoint = "main"; vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; vertexInfo.stage = SDL_GPU_SHADERSTAGE_VERTEX; vertexInfo.num_samplers = 0; vertexInfo.num_storage_buffers = 0; vertexInfo.num_storage_textures = 0; vertexInfo.num_uniform_buffers = 0; - - SDL_ReleaseGPUShader(gpu_wrapper->data(), vertexShader); - vertexShader = SDL_CreateGPUShader(gpu_wrapper->data(), &vertexInfo); + SDL_GPUShader* newVertexShader = SDL_CreateGPUShader(gpu_wrapper->data(), &vertexInfo); + if (newVertexShader == nullptr) { + SDL_LogError(SDL_LOG_CATEGORY_RENDER, "Failed to create vertex shader"); + continue; + } pipelineInfo.vertex_shader = newVertexShader; - SDL_ReleaseGPUGraphicsPipeline(gpu_wrapper->data(), graphicsPipeline); - graphicsPipeline = SDL_CreateGPUGraphicsPipeline(gpu_wrapper->data(), &pipelineInfo); + SDL_GPUGraphicsPipeline* newPipeline = SDL_CreateGPUGraphicsPipeline(gpu_wrapper->data(), &pipelineInfo); + if (newPipeline == nullptr) { + SDL_LogError(SDL_LOG_CATEGORY_RENDER, "Failed to create graphics pipeline"); + SDL_ReleaseGPUShader(gpu_wrapper->data(), newVertexShader); + continue; + } + + // Successfully created new resources, now safe to release old ones + SDL_ReleaseGPUShader(gpu_wrapper->data(), vertexShader); + SDL_ReleaseGPUGraphicsPipeline(gpu_wrapper->data(), graphicsPipeline); + vertexShader = newVertexShader; + graphicsPipeline = newPipeline; }
♻️ Duplicate comments (1)
main.cpp (1)
28-28:pipeline_wrapperis initialized but never used;graphicsPipelineis created separately and used instead.Line 28 initializes
pipeline_wrapperby callinggpu_wrapper->create_pipeline(), but then Line 189 createsgraphicsPipelineseparately with full configuration, and all subsequent operations (binding, recreation, release) usegraphicsPipelinedirectly. This creates two pipeline objects where only one is needed and leaves the wrapper architecture incomplete.Either complete the wrapper integration by removing the raw
graphicsPipelineand usingpipeline_wrapperthroughout, or remove thepipeline_wrapperinitialization on line 28 if the wrapper-based pipeline management is not ready yet.Apply this diff to remove the unused initialization (Option 1):
- std::optional<sopho::PipelineWrapper> pipeline_wrapper{gpu_wrapper->create_pipeline()}; + std::optional<sopho::PipelineWrapper> pipeline_wrapper;Or, if you want to complete the wrapper integration (Option 2), you would need to:
- Remove the
graphicsPipelinemember and all direct SDL pipeline calls- Create and configure
pipeline_wrapperproperly ininit()after setting uppipelineInfo- Update all pipeline operations to use the wrapper's API
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main.cpp(14 hunks)sdl_wrapper/sdl_wrapper.buffer.cpp(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl_wrapper/sdl_wrapper.buffer.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppmain.cpp
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppmain.cpp
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppmain.cpp
🔇 Additional comments (7)
sdl_wrapper/sdl_wrapper.pipeline.cpp (2)
12-18: Constructor implementation looks good.The initialization is straightforward and consistent with the deferred validation pattern used elsewhere in the sdl_wrapper library. Based on learnings.
42-61: Change return type tovoidand add null-pointer guards.Two issues:
Return type should be explicit
void: The function usesautobut doesn't return anything. While this deduces tovoid, it's clearer to writevoidexplicitly for functions that perform side effects without returning a value.Null-pointer dereference risk: Lines 47 and 52 call
m_device->data()without verifying thatm_deviceis valid or thatdata()returns a non-null pointer. This can crash if the device wrapper is invalid.Apply this diff:
- auto PipelineWrapper::submit() + void PipelineWrapper::submit() { - if (modified) + if (modified && m_device && m_device->data()) { modified = false;⛔ Skipped due to learnings
Learnt from: WSQS Repo: WSQS/sdl_test PR: 7 File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43 Timestamp: 2025-11-13T11:35:48.273Z Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.Learnt from: WSQS Repo: WSQS/sdl_test PR: 7 File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0 Timestamp: 2025-11-13T11:34:32.714Z Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.main.cpp (5)
74-85: Comprehensive documentation added.The documentation block accurately describes the initialization responsibilities, including window creation, shader compilation, pipeline setup, buffer uploads, and ImGui initialization.
228-237: Detailed frame processing documentation added.The documentation accurately describes the iterate() responsibilities including UI updates, vertex edits, live shader recompilation, rendering, and command buffer submission.
307-354: Rendering and command buffer management implemented correctly.The command buffer acquisition (Line 307), swapchain texture handling with null check (Lines 315-320), render pass setup, and ImGui integration are all properly implemented. The code correctly submits the command buffer in all paths including early returns.
Note: The static analysis warning at Line 307 about
commandBuffernot being initialized is a false positive—it is initialized by the assignment fromSDL_AcquireGPUCommandBuffer.
356-372: Event handling correctly implemented with clear documentation.The event processing properly forwards events to ImGui and handles window close requests by returning
SDL_APP_SUCCESS. The documentation accurately describes the behavior.
374-400: Cleanup implementation is correct with comprehensive documentation.The shutdown sequence properly releases ImGui resources, GPU shaders and pipeline, releases the window from the GPU device, and destroys the window. The cleanup order is appropriate, and the documentation accurately describes all responsibilities. Wrapper-managed resources (
vertex_buffer,pipeline_wrapper) are automatically cleaned up via RAII.
| PipelineWrapper::~PipelineWrapper() | ||
| { | ||
| if (m_graphics_pipeline) | ||
| { | ||
| SDL_ReleaseGPUGraphicsPipeline(m_device->data(), m_graphics_pipeline); | ||
| m_graphics_pipeline = nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null-pointer guard before calling m_device->data().
The destructor dereferences m_device->data() without verifying that m_device is valid. If the shared pointer is null or data() returns null, this will cause a crash during cleanup.
Consider adding a check:
if (m_graphics_pipeline && m_device && m_device->data())
{
SDL_ReleaseGPUGraphicsPipeline(m_device->data(), m_graphics_pipeline);
m_graphics_pipeline = nullptr;
}🤖 Prompt for AI Agents
In sdl_wrapper/sdl_wrapper.pipeline.cpp around lines 26 to 33, the destructor
calls m_device->data() without ensuring m_device (or its data()) is valid; add a
null-pointer guard so you only call SDL_ReleaseGPUGraphicsPipeline when
m_graphics_pipeline is set and m_device is non-null and m_device->data() is
non-null, then release the pipeline and set m_graphics_pipeline to nullptr.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| module; | ||
| #include <memory> | ||
| #include "SDL3/SDL_gpu.h" | ||
| #include "SDL3/SDL_log.h" |
There was a problem hiding this comment.
warning: included header SDL_gpu.h is not used directly [misc-include-cleaner]
| #include "SDL3/SDL_log.h" | |
| #include "SDL3/SDL_log.h" |
| #include <memory> | ||
| #include "SDL3/SDL_gpu.h" | ||
| #include "SDL3/SDL_log.h" | ||
| #include "shaderc/shaderc.hpp" |
There was a problem hiding this comment.
warning: included header SDL_log.h is not used directly [misc-include-cleaner]
| #include "shaderc/shaderc.hpp" | |
| #include "shaderc/shaderc.hpp" |
| #include "SDL3/SDL_gpu.h" | ||
| #include "SDL3/SDL_log.h" | ||
| #include "shaderc/shaderc.hpp" | ||
| module sdl_wrapper; |
There was a problem hiding this comment.
warning: included header shaderc.hpp is not used directly [misc-include-cleaner]
| module sdl_wrapper; | |
| module sdl_wrapper; |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
sdl_wrapper/sdl_wrapper.pipeline.cpp (1)
58-67: Add m_device validation in destructor.The destructor dereferences
m_devicewithout checking if the shared pointer is valid. Ifm_deviceis null or has been reset, this will cause a crash during cleanup.Apply this diff to add validation:
PipelineWrapper::~PipelineWrapper() { - if (m_graphics_pipeline) + if (m_device && m_graphics_pipeline) { SDL_ReleaseGPUGraphicsPipeline(m_device->data(), m_graphics_pipeline); m_graphics_pipeline = nullptr; } - m_device->release_shader(m_vertex_shader); - m_device->release_shader(m_fragment_shader); + if (m_device) + { + m_device->release_shader(m_vertex_shader); + m_device->release_shader(m_fragment_shader); + } }sdl_wrapper/sdl_wrapper.pipeline.ixx (1)
31-32: Delete copy and move constructors to prevent resource management bugs.The
PipelineWrapperclass manages raw SDL resource pointers (m_graphics_pipeline,m_vertex_shader,m_fragment_shader) via RAII, but lines 31-32 default the copy and move constructors. This enables shallow copying of the raw pointers, which will cause double-free bugs when both the original and the copy are destroyed.Apply this diff to delete the copy and move operations:
public: PipelineWrapper(std::shared_ptr<GpuWrapper> p_device); - PipelineWrapper(const PipelineWrapper&)=default; - PipelineWrapper(PipelineWrapper&&)=default; + PipelineWrapper(const PipelineWrapper&) = delete; + PipelineWrapper& operator=(const PipelineWrapper&) = delete; + PipelineWrapper(PipelineWrapper&&) = delete; + PipelineWrapper& operator=(PipelineWrapper&&) = delete; ~PipelineWrapper();Since
m_deviceis ashared_ptr, the intended lifetime management pattern is forPipelineWrapperinstances to be non-copyable and non-movable, with sharing handled at a higher level if needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(2 hunks)main.cpp(15 hunks)sdl_wrapper/CMakeLists.txt(1 hunks)sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.ixx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cppsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.pipeline.ixx
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cppsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/sdl_wrapper.gpu.ixxsdl_wrapper/sdl_wrapper.pipeline.ixx
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
main.cppCMakeLists.txtsdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/CMakeLists.txtsdl_wrapper/sdl_wrapper.gpu.ixx
🔇 Additional comments (1)
sdl_wrapper/sdl_wrapper.pipeline.cpp (1)
76-98: Add m_device validation in submit method.The method uses
m_device->data()on lines 84 and 89 without verifying the shared pointer is valid. Ifm_deviceis null, this will crash.Apply this diff to add an early guard:
void PipelineWrapper::submit() { + if (!m_device || !m_device->data()) + { + SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s: invalid device", __FUNCTION__); + return; + } + if (m_modified) { m_modified = false; m_pipeline_info.vertex_input_state.vertex_buffer_descriptions = m_vertex_buffer_description.data(); m_pipeline_info.vertex_input_state.vertex_attributes = m_vertex_attribute.data(); m_pipeline_info.target_info.color_target_descriptions = m_color_target_description.data(); auto new_graphics_pipeline = SDL_CreateGPUGraphicsPipeline(m_device->data(), &m_pipeline_info); if (new_graphics_pipeline) { if (m_graphics_pipeline) { SDL_ReleaseGPUGraphicsPipeline(m_device->data(), m_graphics_pipeline); } m_graphics_pipeline = new_graphics_pipeline; } else { SDL_LogError(SDL_LOG_CATEGORY_GPU, "%s get error:%s", __FUNCTION__, SDL_GetError()); } } }⛔ Skipped due to learnings
Learnt from: WSQS Repo: WSQS/sdl_test PR: 7 File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43 Timestamp: 2025-11-13T11:35:48.273Z Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.Learnt from: WSQS Repo: WSQS/sdl_test PR: 7 File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0 Timestamp: 2025-11-13T11:34:32.714Z Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sdl_wrapper/sdl_wrapper.pipeline.cpp (1)
58-67: Add null-pointer guard before accessing m_device.The destructor calls
m_device->data()(line 62) andm_device->release_shader()(lines 65-66) without verifying thatm_deviceis valid. If the shared pointer is null or invalid, this will cause a crash during cleanup.
🧹 Nitpick comments (1)
sdl_wrapper/sdl_wrapper.gpu.ixx (1)
50-63: Consider renamingvertexInfotoshaderInfofor clarity.The variable
vertexInfois used for all shader stages (vertex, fragment, etc.), not just vertex shaders. Renaming it toshaderInfoorcreateInfowould better reflect its generic purpose and improve code readability.Apply this diff:
auto create_shader(const std::vector<uint8_t>& p_shader, SDL_GPUShaderStage p_stage) { - SDL_GPUShaderCreateInfo vertexInfo{}; - vertexInfo.code = p_shader.data(); - vertexInfo.code_size = p_shader.size(); - vertexInfo.entrypoint = "main"; - vertexInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; - vertexInfo.stage = p_stage; - vertexInfo.num_samplers = 0; - vertexInfo.num_storage_buffers = 0; - vertexInfo.num_storage_textures = 0; - vertexInfo.num_uniform_buffers = 0; - return SDL_CreateGPUShader(m_device, &vertexInfo); + SDL_GPUShaderCreateInfo shaderInfo{}; + shaderInfo.code = p_shader.data(); + shaderInfo.code_size = p_shader.size(); + shaderInfo.entrypoint = "main"; + shaderInfo.format = SDL_GPU_SHADERFORMAT_SPIRV; + shaderInfo.stage = p_stage; + shaderInfo.num_samplers = 0; + shaderInfo.num_storage_buffers = 0; + shaderInfo.num_storage_textures = 0; + shaderInfo.num_uniform_buffers = 0; + return SDL_CreateGPUShader(m_device, &shaderInfo); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdl_wrapper/sdl_wrapper.gpu.ixx(1 hunks)sdl_wrapper/sdl_wrapper.pipeline.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-14T00:45:09.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.gpu.ixx
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
sdl_wrapper/sdl_wrapper.pipeline.cppsdl_wrapper/sdl_wrapper.gpu.ixx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: clang-tidy-review
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (windows-latest, Release, gcc)
🔇 Additional comments (8)
sdl_wrapper/sdl_wrapper.pipeline.cpp (4)
14-49: LGTM! Constructor properly initializes pipeline state.The constructor correctly:
- Configures shaderc for Vulkan SPIR-V compilation
- Sets up vertex input descriptions and attributes for position (float3) and color (float4) data
- Configures alpha blending for transparency
- Initializes the pipeline info structure with proper descriptor pointers
76-98: LGTM! Pipeline rebuild logic is correct.The
submit()method properly:
- Guards pipeline recreation behind the modified flag for efficiency
- Updates all descriptor pointers to ensure they reference current vector storage before pipeline creation
- Safely replaces the old pipeline only after the new one is successfully created
- Logs errors with context when pipeline creation fails
100-118: LGTM! Shader compilation and upload is correct.The vertex shader compilation properly:
- Compiles GLSL to SPIR-V using shaderc
- Logs compilation errors with the error message
- Correctly converts SPIR-V data (uint32_t iterator range) to bytes by calculating
(end - begin) * sizeof(uint32_t)for the byte count- Releases the previous shader before creating the new one to avoid leaks
- Marks the pipeline as modified to trigger rebuild
119-137: LGTM! Fragment shader compilation mirrors vertex shader correctly.The implementation properly handles fragment shader compilation using the same correct SPIR-V conversion logic as the vertex shader method, with appropriate shader stage and error logging.
sdl_wrapper/sdl_wrapper.gpu.ixx (4)
14-36: LGTM! Class structure and lifecycle management are correct.The class properly:
- Inherits from
enable_shared_from_thisto supportshared_from_this()calls in factory methods- Initializes the GPU device in the constructor with error logging
- Guards the destructor cleanup with a null check before calling
SDL_DestroyGPUDevice- Resets the device pointer after destruction
The design aligns with the shared_ptr-based ownership model used throughout the wrapper.
48-48: LGTM! Pipeline factory method follows the established pattern.The method correctly returns a
PipelineWrapperconstructed withshared_from_this(), consistent with thecreate_bufferfactory method design.
65-71: LGTM! Shader release is properly guarded.The method correctly validates that the shader pointer is non-null before calling
SDL_ReleaseGPUShader, preventing potential issues from releasing invalid shader handles.
78-81: LGTM! Simple window setter.The method provides a straightforward way to associate the GPU device with a window for swapchain operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
main.cpp (6)
72-83: Aligninit()doc comment with actual return behavior.The doc says
init()may returnSDL_APP_SUCCESS, but the implementation only ever returnsSDL_APP_CONTINUE. Either document that onlySDL_APP_CONTINUEis used today or add explicit success/failure paths to match the comment.
88-92: Consider handling failure fromSDL_ClaimWindowForGPUDevicebefore using the pipeline.Right now you call
SDL_ClaimWindowForGPUDevice(gpu_wrapper->data(), window);and immediatelypipeline_wrapper.emplace(gpu_wrapper);without checking for failure. IfSDL_ClaimWindowForGPUDevicecan fail, it would be safer to:
- Check its return value.
- Bail out (or log and return an appropriate
SDL_AppResult) instead of proceeding to create and use a pipeline bound to an unclaimed window.
119-128: Swapchain format and blending config are sensible; consider reusing the format value.Configuring blending with
SDL_GPU_BLENDOP_ADDand enabling blend, then usingSDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), window)both for the color target description and ImGui’sinit_info.ColorTargetFormat, keeps everything consistent with the swapchain.If you want to reduce duplication and repeated calls, you could cache the format once:
- colorTargetDescriptions[0].format = SDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), window); + const auto swapchain_format = SDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), window); + colorTargetDescriptions[0].format = swapchain_format; ... - init_info.ColorTargetFormat = SDL_GetGPUSwapchainTextureFormat(gpu_wrapper->data(), window); + init_info.ColorTargetFormat = swapchain_format;Also applies to: 166-167
133-137:pipelineInfoappears unused after switching toPipelineWrapper.With the direct call to
SDL_CreateGPUGraphicsPipelinecommented out,pipelineInfois only written to and never read. That’s dead state unlessPipelineWrapperaccesses it indirectly, which doesn’t happen here.Either:
- Remove
pipelineInfoand the associated setup ifPipelineWrapperfully owns pipeline configuration now, or- Expose a small API on
PipelineWrapperto accept the vertex input/target configuration instead of leaving this member unused.
213-213: EnsurePipelineWrapper::submit()is incremental, or gate it on shader edits.In
iterate()you:
- Mark the vertex shader as changed on text edits (
set_vertex_shader(vertex_source)), and- Call
pipeline_wrapper->submit();every frame, regardless of whether anything changed.This is fine if
submit()internally tracks a “dirty” flag and only recompiles/recreates the pipeline when necessary. If it doesn’t, you may be recompiling every frame, which would be unnecessarily expensive; in that case, consider callingsubmit()only when the shader source actually changed.Also applies to: 222-223
292-300: Updatequit()documentation to reflect RAII-based GPU resource cleanup.The comment still talks about explicitly releasing shaders and the graphics pipeline, but
quit()now only shuts down ImGui, releases the window from the GPU device, and destroys the window; shader/pipeline cleanup is presumably handled by the wrappers’ destructors.Consider rewording the docblock to emphasize:
- ImGui backend shutdown,
- Window/device unbinding,
- And that GPU resources are released via
GpuWrapper,BufferWrapper, andPipelineWrapperlifetimes.Also applies to: 307-307
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(12 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
📚 Learning: 2025-11-13T11:35:48.273Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:37-43
Timestamp: 2025-11-13T11:35:48.273Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, null pointer validation for the GpuWrapper::create_buffer method (checking m_device, SDL_CreateGPUBuffer return value, and shared_from_this() validity) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-14T00:45:09.377Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:73-76
Timestamp: 2025-11-14T00:45:09.377Z
Learning: In sdl_wrapper/sdl_wrapper.gpu.ixx, validation for m_window in the GpuWrapper::get_texture_formate() method (checking if m_window is nullptr before calling SDL_GetGPUSwapchainTextureFormat) is deferred to a future window wrapper implementation and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-13T11:34:32.714Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.buffer.ixx:0-0
Timestamp: 2025-11-13T11:34:32.714Z
Learning: In sdl_wrapper/sdl_wrapper.buffer.ixx, null pointer validation for the BufferWrapper constructor (checking p_gpu, p_gpu->data(), and SDL_CreateGPUBuffer return value) is deferred to a future issue and should not be flagged in reviews.
Applied to files:
main.cpp
📚 Learning: 2025-11-13T02:58:26.859Z
Learnt from: WSQS
Repo: WSQS/sdl_test PR: 7
File: sdl_wrapper/sdl_wrapper.gpu.ixx:0-0
Timestamp: 2025-11-13T02:58:26.859Z
Learning: The sdl_wrapper library (in the repository WSQS/sdl_test) does not allow throwing exceptions. Error handling must use return codes, validity checks, or other non-exception mechanisms.
Applied to files:
main.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, gcc)
🔇 Additional comments (6)
main.cpp (6)
12-13: SDL GPU/log headers now align with usage and fix include warnings.Including
<SDL3/SDL_gpu.h>and"SDL3/SDL_log.h"matches the use of GPU enums/types and log categories and should resolve the previous include-cleaner diagnostics.
26-31: Wrapper ownership and construction order look consistent.Creating
gpu_wrapperandvertex_bufferas members, withpipeline_wrapperoptional and the legacygraphicsPipelinecommented out, is coherent with the current wrapper-based design and the fixed SDL initialization order.
93-97: Vertex buffer layout configuration is straightforward and correct.
pitch = sizeof(Vertex)andbuffer_slot = 0match the single bound vertex buffer and theVertexstruct layout; this is a clear, idiomatic setup.Also applies to: 103-107
176-185:iterate()doc comment matches the implemented per-frame responsibilities.The description (UI update, optional vertex upload, live shader recompilation, rendering, and command buffer submission) accurately reflects what
iterate()does now.
225-225: Render loop now cleanly uses wrapper-managed GPU handles.Acquiring the command buffer via
gpu_wrapper->data(), bindingpipeline_wrapper->data()as the graphics pipeline, and usingvertex_buffer.data()in the buffer binding keeps the render pass fully wrapper-backed. The resource usage pattern here looks correct.Also applies to: 253-257
274-279:event()doc comment and behavior are consistent.The comment correctly matches the implementation: events are forwarded to ImGui, and a window close request returns
SDL_APP_SUCCESS, otherwiseSDL_APP_CONTINUE.
| bufferInfo.size = sizeof(vertices); | ||
| bufferInfo.usage = SDL_GPU_BUFFERUSAGE_VERTEX; | ||
| vertexBuffer.emplace(device, &bufferInfo); | ||
| vertex_buffer.upload(&vertices, sizeof(vertices), 0); |
There was a problem hiding this comment.
Use vertices.data() instead of &vertices when uploading to the GPU buffer.
&vertices is a pointer to the std::array object, not explicitly to the first Vertex. While it is likely to work in practice, using the element pointer is clearer and safer:
- vertex_buffer.upload(&vertices, sizeof(vertices), 0);
+ vertex_buffer.upload(vertices.data(), sizeof(vertices), 0);Apply this in both init() and iterate() where you upload the vertex data.
Also applies to: 201-201
🤖 Prompt for AI Agents
In main.cpp around line 138 (and also at line 201), the upload call passes
&vertices (pointer to the std::array object) instead of a pointer to the first
Vertex; change the upload calls to use vertices.data() as the source pointer so
the GPU receives the array element data. Update both occurrences (init() and
iterate()) to call vertex_buffer.upload(vertices.data(), sizeof(vertices), 0)
ensuring the first- element pointer is passed.
|
|
||
| #include <SDL3/SDL_gpu.h> | ||
| #include "SDL3/SDL_log.h" | ||
| import sdl_wrapper; |
There was a problem hiding this comment.
warning: included header SDL_log.h is not used directly [misc-include-cleaner]
| import sdl_wrapper; | |
| import sdl_wrapper; |
Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_wrapper.buffer.cpp` * `sdl_wrapper/sdl_wrapper.pipeline.cpp`
* 📝 Add docstrings to `pipiline` Docstrings generation was requested by @WSQS. * #7 (comment) The following files were modified: * `main.cpp` * `sdl_wrapper/sdl_wrapper.buffer.cpp` * `sdl_wrapper/sdl_wrapper.pipeline.cpp` * style: format code with ClangFormat This commit fixes the style issues introduced in 66165a8 according to the output from ClangFormat. Details: #19 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation