Include clang-format document#29
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (73)
📝 WalkthroughWalkthroughAdds a repository .clang-format, updates .clang-format-ignore, introduces an exported core.containers.string module, and reformats many source and module files to C++20 module style with consistent const/ref qualifiers, brace/whitespace normalization, and no substantive API or behavior changes. ChangesFormatting & Module Conversion
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.clang-format (1)
1-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate
.clang-formatoption and validate the file
- Remove the duplicate key
AllowShortCaseExpressionOnASingleLine: true(it appears twice).- Reword informal/unprofessional comments in the config (e.g., “Microslop BS”, “Pain in the ass”).
- Run
clang-format --style=file --dry-run --Werror <some .cpp/.h file>with the sameclang-formatversion used in CI to catch any remaining option/value issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.clang-format around lines 1 - 208, Remove the duplicated key AllowShortCaseExpressionOnASingleLine (leave a single occurrence) and replace the informal comments "Microslop BS \r\n" and "Pain in the ass" with professional wording (e.g., "CRLF line endings not allowed" and "Avoid unbraced statements; prefer braces for clarity") in the .clang-format content; then validate the file by running clang-format with the CI version: clang-format --style=file --dry-run --Werror <example source file> and fix any remaining invalid options or values reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.clang-format:
- Around line 37-38: The .clang-format contains a duplicated option key; remove
the redundant line so that AllowShortCaseExpressionOnASingleLine appears only
once—locate the consecutive duplicate entries for
AllowShortCaseExpressionOnASingleLine and delete one of them to keep a single
setting.
- Line 63: The .clang-format entry uses the incorrect key name
BraceWrappingAfterControlStatementStyle; update the configuration to use the
proper clang-format YAML key by moving this setting under BraceWrapping with the
AfterControlStatement field (e.g. set BraceWrapping: { AfterControlStatement:
BWACS_Never } or equivalent YAML mapping) so the value BWACS_Never is applied;
look for the existing BraceWrappingAfterControlStatementStyle occurrence and
replace it with the BraceWrapping -> AfterControlStatement mapping.
- Line 151: Replace the unprofessional comment string "# No Microslop BS" in the
.clang-format file with a neutral, professional phrase (or remove it entirely);
locate the comment by searching for the exact text "# No Microslop BS" and
update it to something like a concise, descriptive note about the intent (e.g.,
indicating that project-specific or proprietary vendor settings are excluded) to
keep the config file professional.
- Line 109: The .clang-format sets Cpp11BracedListStyle to an invalid value
"BLS_FunctionCall"; update the Cpp11BracedListStyle entry to a boolean (set
Cpp11BracedListStyle: true or Cpp11BracedListStyle: false) and, if you intended
specific braced-list spacing/line-breaking behavior, add the appropriate
braced-list options such as BreakAfterOpenBracketBracedList or
AllowShortBracedLists to express the desired formatting; locate the
Cpp11BracedListStyle key in the file and replace the current token with the
correct boolean and any supplemental braced-list options.
---
Outside diff comments:
In @.clang-format:
- Around line 1-208: Remove the duplicated key
AllowShortCaseExpressionOnASingleLine (leave a single occurrence) and replace
the informal comments "Microslop BS \r\n" and "Pain in the ass" with
professional wording (e.g., "CRLF line endings not allowed" and "Avoid unbraced
statements; prefer braces for clarity") in the .clang-format content; then
validate the file by running clang-format with the CI version: clang-format
--style=file --dry-run --Werror <example source file> and fix any remaining
invalid options or values reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| KeepFormFeed: false | ||
|
|
||
| # No Microslop BS \r\n |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Revise unprofessional comment.
The comment contains informal language and a derogatory reference that is inappropriate for a project configuration file. Consider rewording to maintain professionalism.
📝 Suggested revision
-# No Microslop BS \r\n
+# Use Unix-style line endings (LF) instead of Windows-style (CRLF)
LineEnding: LF📝 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.
| # No Microslop BS \r\n | |
| # Use Unix-style line endings (LF) instead of Windows-style (CRLF) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.clang-format at line 151, Replace the unprofessional comment string "# No
Microslop BS" in the .clang-format file with a neutral, professional phrase (or
remove it entirely); locate the comment by searching for the exact text "# No
Microslop BS" and update it to something like a concise, descriptive note about
the intent (e.g., indicating that project-specific or proprietary vendor
settings are excluded) to keep the config file professional.
OldDev78
left a comment
OldDev78
left a comment
There was a problem hiding this comment.
I'd recommend applying the changes to our source in this PR once you're ready to merge. We'll know all of the whitespace changes spam is for formatting.
| Standard: Cpp23 | ||
|
|
||
| # Pointer and reference alignment style. Possible values: Left, Right, Middle. | ||
| PointerAlignment: Right |
There was a problem hiding this comment.
Vote for pointer to the left (on the side of the type)
| AfterNamespace: false | ||
| AfterStruct: false | ||
| AfterExtern: false | ||
| BeforeCatch: true # catch on separate line, not like we'll use catch |
There was a problem hiding this comment.
Just for consistency. I'd also rather have K&R
| AfterStruct: false | ||
| AfterExtern: false | ||
| BeforeCatch: true # catch on separate line, not like we'll use catch | ||
| BeforeElse: true |
There was a problem hiding this comment.
Not having a break for if but having for else will look strange for me.
There was a problem hiding this comment.
This looks like
if(false) {
// Stuff
}
else if (true) {
}
if you set BeforeElse to false it would like
if(false) {
// Stuff
} else if (true) {
}
In my opinion that does make it a little harder to read especially if the last line before the else has a lot of values.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/native/core/io/image_loader.cpp (1)
8-9:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBuild failure: stb_image.h is incompatible with C++20 module scanning.
The pipeline logs show a critical preprocessor error when clang-scan-deps attempts to scan stb_image.h. Legacy single-header libraries with complex preprocessor logic cannot be included in module purview (global module fragment or otherwise) because the module dependency scanner must parse them.
Move the stb implementation to a separate non-module translation unit to isolate it from the module system.
🔧 Recommended fix: separate stb into a non-module TU
Create
engine/native/core/io/image_loader_stb.cpp(non-module file):`#define` STB_IMAGE_IMPLEMENTATION `#include` <stb_image.h> `#include` <filesystem> namespace draco::core::io::image_loader::detail { unsigned char* stb_load_wrapper(const char* path, int* width, int* height, int* channels) { return stbi_load(path, width, height, channels, STBI_rgb_alpha); } void stb_free_wrapper(unsigned char* data) { stbi_image_free(data); } } // namespace draco::core::io::image_loader::detailThen update this file to remove the stb include from the global fragment and forward-declare the wrapper:
module; `#include` <vector> `#include` <filesystem> `#include` <limits> `#include` <print> - -#define STB_IMAGE_IMPLEMENTATION -#include <stb_image.h> module core.io.image_loader; import core.stdtypes; +namespace draco::core::io::image_loader::detail { +extern unsigned char* stb_load_wrapper(const char* path, int* width, int* height, int* channels); +extern void stb_free_wrapper(unsigned char* data); +} + // TODO: I'm too lazy to write code so we need somethin' better namespace draco::core::io::image_loader {And replace calls to
stbi_loadandstbi_image_freewith the wrapper functions:- unsigned char *data = stbi_load(path.string().c_str(), &width, &height, &channels, STBI_rgb_alpha); + unsigned char *data = detail::stb_load_wrapper(path.string().c_str(), &width, &height, &channels); if (!data) { std::println("Error: Failed to decode image: {}", path.string()); @@ -34,7 +38,7 @@ } if (width <= 0 || height <= 0) { - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; } @@ -42,7 +46,7 @@ usize const w = static_cast<usize>(width); usize const h = static_cast<usize>(height); if (w > (std::numeric_limits<usize>::max() / 4) / h) { - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; } @@ -55,7 +59,7 @@ result.is_valid = true; // Free the memory allocated by stb - stbi_image_free(data); + detail::stb_free_wrapper(data); return result; }Add the new file to CMakeLists.txt as a regular (non-module) source file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/native/core/io/image_loader.cpp` around lines 8 - 9, stb_image's implementation must be moved out of the C++20 module TU: create a new non-module translation unit (e.g., image_loader_stb.cpp) that defines STB_IMAGE_IMPLEMENTATION, includes <stb_image.h>, and implements two C++ wrappers in namespace draco::core::io::image_loader::detail (e.g., stb_load_wrapper(const char* path, int* w,int* h,int* c) returning unsigned char* and stb_free_wrapper(unsigned char*)): replace direct calls to stbi_load/stbi_image_free in image_loader.cpp with these wrapper functions and add a forward declaration for the wrappers in image_loader.cpp, then add image_loader_stb.cpp to the build (CMakeLists) as a normal non-module source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/native/core/math/vector4.cppm`:
- Around line 66-90: The consteval branches of Vector4::operator[](i32) and the
const overload currently map any invalid index to w; change those default cases
to reject invalid indices (so bad constant indices fail to compile) by throwing
an out-of-range exception (e.g. throw std::out_of_range("Vector4 index out of
range")) or otherwise forcing a compile-time error instead of returning w; leave
the non-consteval path (return (&x)[i]) unchanged and apply the same change to
both Vector4::operator[](i32) and Vector4::operator[](i32) const to ensure
consistent behavior in constexpr evaluation.
In `@engine/native/rendering/mesh/mesh.cpp`:
- Around line 68-81: The functions (e.g., create_cube) currently insert the
returned MeshHandle from create(...) into g_mesh_cache unconditionally, which
caches failed/empty handles; change the logic to only insert into g_mesh_cache
after verifying the handle is valid/non-empty (check the MeshHandle result of
create(...) before assigning g_mesh_cache[key] = h and returning), and apply the
same fix to the other factory functions mentioned (the blocks around the other
similar create_* functions) so failed creations are not cached.
In `@engine/native/rendering/rhi/commands.cpp`:
- Around line 53-64: apply_view currently skips calling bgfx::setViewFrameBuffer
when desc.fb == InvalidFramebuffer, leaving the view's framebuffer sticky and
possibly redirecting later draws; update apply_view so that when desc.fb ==
InvalidFramebuffer you explicitly call bgfx::setViewFrameBuffer(view,
BGFX_INVALID_HANDLE) otherwise resolve the framebuffer handle via
get_checked(g_framebuffers, desc.fb) and call bgfx::setViewFrameBuffer(view,
fb->fbh) as before, preserving the existing RHI_WARN path for invalid handles.
In `@engine/native/rendering/rhi/core.cpp`:
- Around line 127-158: The shutdown() function currently only destroys live
registries and calls bgfx::shutdown(), but skips items already moved into
g_deletion_queue and never releases g_framebuffers; before calling
bgfx::shutdown() flush g_deletion_queue by iterating its entries and performing
the appropriate bgfx::destroy on stored handles (matching the same destroy logic
used in destroy_buffer()/destroy_uniform()), clear the queue, and add a loop
over g_framebuffers.internal().raw() to destroy any live framebuffer handles
(similar to the g_textures/g_uniforms loops). Ensure these flushes happen prior
to bgfx::shutdown() so all deferred native destroys run.
In `@engine/native/rendering/rhi/rhi.cppm`:
- Around line 114-121: The Buffer struct's DynamicIndexBufferHandle member dibh
is not initialized which can leave it indeterminate; modify struct Buffer so
dibh is default-initialized to BGFX_INVALID_HANDLE (matching vbh, dvbh, ibh) to
ensure safe default construction and proper cleanup/update paths, i.e. update
the declaration of dibh inside struct Buffer to use BGFX_INVALID_HANDLE.
In `@engine/native/rendering/rhi/texture.cpp`:
- Around line 54-60: create_framebuffer currently ignores the requested
TextureFormat and always uses bgfx::TextureFormat::RGBA8; change it to map the
incoming TextureFormat to the appropriate bgfx::TextureFormat::Enum before
creating the texture. Update create_framebuffer to convert TextureFormat (the
enum parameter) into a bgfx_format via a switch or small helper (e.g.,
mapTextureFormatToBgfx) covering all supported formats and providing a sensible
fallback (or assert/log) for unsupported values, then pass that bgfx_format into
bgfx::createTexture2D instead of the hardcoded RGBA8.
---
Outside diff comments:
In `@engine/native/core/io/image_loader.cpp`:
- Around line 8-9: stb_image's implementation must be moved out of the C++20
module TU: create a new non-module translation unit (e.g., image_loader_stb.cpp)
that defines STB_IMAGE_IMPLEMENTATION, includes <stb_image.h>, and implements
two C++ wrappers in namespace draco::core::io::image_loader::detail (e.g.,
stb_load_wrapper(const char* path, int* w,int* h,int* c) returning unsigned
char* and stb_free_wrapper(unsigned char*)): replace direct calls to
stbi_load/stbi_image_free in image_loader.cpp with these wrapper functions and
add a forward declaration for the wrappers in image_loader.cpp, then add
image_loader_stb.cpp to the build (CMakeLists) as a normal non-module source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7264fcd0-e9da-40d0-874a-b4c0f4b66d0f
📒 Files selected for processing (74)
.clang-format.clang-format-ignoreengine/native/core/containers/string.cppmengine/native/core/definitions/definitions.cppmengine/native/core/definitions/stdtypes.cppmengine/native/core/definitions/version.cppmengine/native/core/io/filesystem.cppengine/native/core/io/filesystem.cppmengine/native/core/io/image_loader.cppengine/native/core/io/image_loader.cppmengine/native/core/io/io.cppmengine/native/core/math/constants.cppmengine/native/core/math/functions.cppmengine/native/core/math/math.test.cppengine/native/core/math/transform.cppengine/native/core/math/transform.cppmengine/native/core/math/types.cppmengine/native/core/math/types_common.cppmengine/native/core/math/vector2.cppmengine/native/core/math/vector3.cppmengine/native/core/math/vector4.cppmengine/native/core/memory/allocator.cppengine/native/core/memory/allocator.cppmengine/native/core/memory/bumpAllocator.cppengine/native/core/memory/bumpAllocator.cppmengine/native/core/memory/bumpAllocator.test.cppengine/native/core/memory/fixedAllocator.cppengine/native/core/memory/fixedAllocator.cppmengine/native/core/memory/fixedAllocator.test.cppengine/native/core/memory/handle.cppmengine/native/core/memory/handle_registry.cppmengine/native/core/memory/pageAllocator.cppengine/native/core/memory/pageAllocator.cppmengine/native/core/memory/slice.cppmengine/native/core/memory/slot_array.cppmengine/native/core/memory/trackingAllocator.cppengine/native/core/memory/trackingAllocator.cppmengine/native/core/memory/trackingAllocator.test.cppengine/native/input/input.cppengine/native/input/input.cppmengine/native/main/main.cppengine/native/platform/cpu/cpu_info.hengine/native/platform/cpu/cpu_info_neon.cppengine/native/platform/cpu/cpu_info_x64.cppengine/native/platform/linux/linux.cppengine/native/platform/platform.cppmengine/native/platform/simd.hengine/native/platform/win32/win32.cppengine/native/rendering/material/material.cppmengine/native/rendering/mesh/mesh.cppengine/native/rendering/mesh/mesh.cppmengine/native/rendering/quad_renderer/quad_renderer.cppengine/native/rendering/quad_renderer/quad_renderer.cppmengine/native/rendering/renderer/renderer.cppengine/native/rendering/renderer/renderer.cppmengine/native/rendering/rendergraph/rendergraph.cppengine/native/rendering/rendergraph/rendergraph.cppmengine/native/rendering/rhi/buffers.cppengine/native/rendering/rhi/commands.cppengine/native/rendering/rhi/core.cppengine/native/rendering/rhi/macros.hengine/native/rendering/rhi/pipelines.cppengine/native/rendering/rhi/rhi.cppmengine/native/rendering/rhi/texture.cppengine/native/rendering/rhi/uniform_registry.cppmengine/native/rendering/rhi/vertex.cppmengine/native/scene/camera/camera_controller.cppengine/native/scene/camera/camera_controller.cppmengine/native/scene/renderable/renderable.cppmengine/native/scene/scene.cppmengine/native/scene/transform_component/transform_component.cppengine/native/scene/transform_component/transform_component.cppmengine/native/thirdparty/doctest/doctest.hengine/native/thirdparty/stb/stb_image.h
✅ Files skipped from review due to trivial changes (51)
- .clang-format-ignore
- engine/native/platform/cpu/cpu_info.h
- engine/native/scene/transform_component/transform_component.cpp
- engine/native/scene/scene.cppm
- engine/native/core/io/filesystem.cppm
- engine/native/core/math/types.cppm
- engine/native/core/math/transform.cppm
- engine/native/core/memory/handle.cppm
- engine/native/core/memory/fixedAllocator.test.cpp
- engine/native/core/containers/string.cppm
- engine/native/core/memory/allocator.cppm
- engine/native/core/memory/slice.cppm
- engine/native/platform/cpu/cpu_info_neon.cpp
- engine/native/core/memory/trackingAllocator.test.cpp
- engine/native/core/io/io.cppm
- engine/native/core/definitions/definitions.cppm
- engine/native/scene/camera/camera_controller.cppm
- engine/native/platform/platform.cppm
- engine/native/scene/transform_component/transform_component.cppm
- engine/native/platform/simd.h
- engine/native/rendering/material/material.cppm
- .clang-format
- engine/native/scene/renderable/renderable.cppm
- engine/native/rendering/rhi/uniform_registry.cppm
- engine/native/core/io/image_loader.cppm
- engine/native/core/memory/handle_registry.cppm
- engine/native/platform/linux/linux.cpp
- engine/native/input/input.cppm
- engine/native/core/memory/trackingAllocator.cppm
- engine/native/core/memory/pageAllocator.cppm
- engine/native/rendering/rhi/vertex.cppm
- engine/native/core/memory/fixedAllocator.cppm
- engine/native/rendering/rhi/macros.h
- engine/native/core/definitions/version.cppm
- engine/native/rendering/renderer/renderer.cppm
- engine/native/rendering/rendergraph/rendergraph.cpp
- engine/native/rendering/rendergraph/rendergraph.cppm
- engine/native/core/definitions/stdtypes.cppm
- engine/native/core/memory/fixedAllocator.cpp
- engine/native/core/memory/trackingAllocator.cpp
- engine/native/platform/cpu/cpu_info_x64.cpp
- engine/native/core/memory/bumpAllocator.test.cpp
- engine/native/rendering/rhi/buffers.cpp
- engine/native/platform/win32/win32.cpp
- engine/native/core/math/functions.cppm
- engine/native/core/math/vector3.cppm
- engine/native/core/io/filesystem.cpp
- engine/native/rendering/quad_renderer/quad_renderer.cppm
- engine/native/rendering/quad_renderer/quad_renderer.cpp
- engine/native/core/math/math.test.cpp
- engine/native/core/math/vector2.cppm
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | ||
| if consteval { | ||
| switch (i) { | ||
| case 0: return x; | ||
| case 1: return y; | ||
| case 2: return z; | ||
| default: | ||
| case 3: return w; | ||
| } | ||
| } | ||
| else { return (&x)[i]; } | ||
| } | ||
|
|
||
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | ||
| if consteval { | ||
| switch (i) { | ||
| case 0: return x; | ||
| case 1: return y; | ||
| case 2: return z; | ||
| default: | ||
| case 3: return w; | ||
| } | ||
| } | ||
| else { return (&x)[i]; } | ||
| } |
There was a problem hiding this comment.
Reject invalid constexpr indices instead of aliasing them to w.
In the if consteval path, any index other than 0/1/2 currently falls into default and returns w. That means v[4] in a constant-evaluated context produces the wrong component instead of surfacing the bug, while the swizzle helpers already reject bad indices.
Suggested fix
[[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept {
if consteval {
switch (i) {
case 0: return x;
case 1: return y;
case 2: return z;
- default:
case 3: return w;
+ default: throw "Index out of range";
}
}
else { return (&x)[i]; }
}
[[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept {
if consteval {
switch (i) {
case 0: return x;
case 1: return y;
case 2: return z;
- default:
case 3: return w;
+ default: throw "Index out of range";
}
}
else { return (&x)[i]; }
}📝 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.
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| default: | |
| case 3: return w; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| default: | |
| case 3: return w; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 &Vector4::operator[](i32 const i) noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| case 3: return w; | |
| default: throw "Index out of range"; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } | |
| [[nodiscard]] constexpr f32 const &Vector4::operator[](i32 const i) const noexcept { | |
| if consteval { | |
| switch (i) { | |
| case 0: return x; | |
| case 1: return y; | |
| case 2: return z; | |
| case 3: return w; | |
| default: throw "Index out of range"; | |
| } | |
| } | |
| else { return (&x)[i]; } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/core/math/vector4.cppm` around lines 66 - 90, The consteval
branches of Vector4::operator[](i32) and the const overload currently map any
invalid index to w; change those default cases to reject invalid indices (so bad
constant indices fail to compile) by throwing an out-of-range exception (e.g.
throw std::out_of_range("Vector4 index out of range")) or otherwise forcing a
compile-time error instead of returning w; leave the non-consteval path (return
(&x)[i]) unchanged and apply the same change to both Vector4::operator[](i32)
and Vector4::operator[](i32) const to ensure consistent behavior in constexpr
evaluation.
| MeshHandle create_cube() { | ||
| ensure_mesh_layout(); | ||
|
|
||
| mesh.vertex_count = vertex_count; | ||
| mesh.index_count = static_cast<u32>(indices.size()); | ||
| usize key = 1; | ||
|
|
||
| mesh.valid = (mesh.vbh != rhi::InvalidBuffer) && (mesh.ibh != rhi::InvalidBuffer); | ||
| if (auto it = g_mesh_cache.find(key); it != g_mesh_cache.end()) { return it->second; } | ||
|
|
||
| if (!mesh.valid) | ||
| { | ||
| if (mesh.vbh != rhi::InvalidBuffer) rhi::destroy_buffer(mesh.vbh); | ||
| if (mesh.ibh != rhi::InvalidBuffer) rhi::destroy_buffer(mesh.ibh); | ||
| return {}; | ||
| } | ||
| return g_meshes.create(mesh); | ||
| } | ||
| auto v = gen::cube_vertices(); | ||
| auto i = gen::cube_indices(); | ||
|
|
||
| MeshHandle create_cube() | ||
| { | ||
| ensure_mesh_layout(); | ||
| MeshHandle h = create(v.data(), v.size() * sizeof(Vertex), (u32)v.size(), i, g_mesh_layout); | ||
|
|
||
| usize key = 1; | ||
| g_mesh_cache[key] = h; | ||
| return h; |
There was a problem hiding this comment.
Don't cache failed mesh creations.
Each factory stores h in g_mesh_cache even when create(...) returns an empty handle. That makes a transient allocation/init failure permanent for that key, because every later call will hit the cache and immediately return the same invalid handle. Only populate the cache after confirming the mesh creation succeeded.
Also applies to: 84-97, 100-115, 118-133, 136-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/mesh/mesh.cpp` around lines 68 - 81, The functions
(e.g., create_cube) currently insert the returned MeshHandle from create(...)
into g_mesh_cache unconditionally, which caches failed/empty handles; change the
logic to only insert into g_mesh_cache after verifying the handle is
valid/non-empty (check the MeshHandle result of create(...) before assigning
g_mesh_cache[key] = h and returning), and apply the same fix to the other
factory functions mentioned (the blocks around the other similar create_*
functions) so failed creations are not cached.
| void apply_view(ViewID view, ViewDesc const &desc) { | ||
| if (desc.fb != InvalidFramebuffer) { | ||
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | ||
|
|
||
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | ||
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | ||
| } | ||
|
|
||
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | ||
|
|
||
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In bgfx, do per-view settings like the framebuffer remain in effect until bgfx::setViewFrameBufferis called again, and isBGFX_INVALID_HANDLE the correct way to restore rendering to the default backbuffer?
💡 Result:
In bgfx, per-view settings like those configured via bgfx::setViewFrameBuffer are persistent throughout the duration of a single frame [1][2]. Because bgfx views are stateful containers that persist until the next call to bgfx::frame, any view configuration (such as the assigned framebuffer) remains in effect for all subsequent draw calls submitted to that same view ID within the same frame [1][2]. If you call bgfx::setViewFrameBuffer multiple times for the same view ID before bgfx::frame is called, only the final configuration is respected [1][2]. Regarding the default backbuffer, yes, BGFX_INVALID_HANDLE is the correct handle to use with bgfx::setViewFrameBuffer to restore or set the rendering target to the default backbuffer [3][4].
Citations:
- 1: Question about view ids bkaradzic/bgfx#1054
- 2: How to setViewTransform for each time bgfx::submit bkaradzic/bgfx#1673
- 3: https://bkaradzic.github.io/bgfx/bgfx.html
- 4: https://docs.rs/bgfx-sys/latest/bgfx_sys/fn.bgfx_set_view_frame_buffer.html
Reset the bgfx view framebuffer when desc.fb is invalid.
apply_view only calls bgfx::setViewFrameBuffer for non-invalid desc.fb; bgfx view framebuffer state is sticky for subsequent draws in the frame, so reusing a view that previously targeted an offscreen framebuffer can cause later passes to render to the wrong target. Use BGFX_INVALID_HANDLE when desc.fb == InvalidFramebuffer.
🐛 Proposed fix
void apply_view(ViewID view, ViewDesc const &desc) {
if (desc.fb != InvalidFramebuffer) {
auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer");
if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); }
else { RHI_WARN(false, "Framebuffer invalid at apply_view"); }
}
+ else {
+ bgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE);
+ }
bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h);
if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); }
}📝 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.
| void apply_view(ViewID view, ViewDesc const &desc) { | |
| if (desc.fb != InvalidFramebuffer) { | |
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | |
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | |
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | |
| } | |
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | |
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | |
| } | |
| void apply_view(ViewID view, ViewDesc const &desc) { | |
| if (desc.fb != InvalidFramebuffer) { | |
| auto *fb = get_checked(g_framebuffers, desc.fb, "Framebuffer"); | |
| if (fb && bgfx::isValid(fb->fbh)) { bgfx::setViewFrameBuffer(view, fb->fbh); } | |
| else { RHI_WARN(false, "Framebuffer invalid at apply_view"); } | |
| } | |
| else { | |
| bgfx::setViewFrameBuffer(view, BGFX_INVALID_HANDLE); | |
| } | |
| bgfx::setViewRect(view, desc.x, desc.y, desc.w, desc.h); | |
| if (desc.clear_flags != 0) { bgfx::setViewClear(view, desc.clear_flags, desc.clear_color); } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/rhi/commands.cpp` around lines 53 - 64, apply_view
currently skips calling bgfx::setViewFrameBuffer when desc.fb ==
InvalidFramebuffer, leaving the view's framebuffer sticky and possibly
redirecting later draws; update apply_view so that when desc.fb ==
InvalidFramebuffer you explicitly call bgfx::setViewFrameBuffer(view,
BGFX_INVALID_HANDLE) otherwise resolve the framebuffer handle via
get_checked(g_framebuffers, desc.fb) and call bgfx::setViewFrameBuffer(view,
fb->fbh) as before, preserving the existing RHI_WARN path for invalid handles.
| void shutdown() { | ||
| // Walk all registries and destroy live GPU objects | ||
| for (auto &slot : g_buffers.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value.vbh)) { bgfx::destroy(slot.value.vbh); } | ||
|
|
||
| if (bgfx::isValid(slot.value.ibh)) { bgfx::destroy(slot.value.ibh); } | ||
|
|
||
| if (bgfx::isValid(slot.value.dvbh)) { bgfx::destroy(slot.value.dvbh); } | ||
| } | ||
|
|
||
| for (auto &slot : g_pipelines.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value.program)) { bgfx::destroy(slot.value.program); } | ||
| } | ||
|
|
||
| for (auto &slot : g_uniforms.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value)) { bgfx::destroy(slot.value); } | ||
| } | ||
|
|
||
| for (auto &slot : g_textures.internal().raw()) { | ||
| if (!slot.alive) { continue; } | ||
|
|
||
| if (bgfx::isValid(slot.value)) { bgfx::destroy(slot.value); } | ||
| } | ||
|
|
||
| bgfx::shutdown(); | ||
| } |
There was a problem hiding this comment.
Flush deferred deletions and destroy framebuffers during shutdown.
shutdown() only walks live registry slots. Anything already passed through destroy_buffer()/destroy_uniform() has been removed from its registry and now exists only in g_deletion_queue, so it is skipped here and bgfx::shutdown() runs before those native destroys ever happen. g_framebuffers is also never drained in this path. Please flush the queued deletions and explicitly release live framebuffers before shutdown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/rhi/core.cpp` around lines 127 - 158, The shutdown()
function currently only destroys live registries and calls bgfx::shutdown(), but
skips items already moved into g_deletion_queue and never releases
g_framebuffers; before calling bgfx::shutdown() flush g_deletion_queue by
iterating its entries and performing the appropriate bgfx::destroy on stored
handles (matching the same destroy logic used in
destroy_buffer()/destroy_uniform()), clear the queue, and add a loop over
g_framebuffers.internal().raw() to destroy any live framebuffer handles (similar
to the g_textures/g_uniforms loops). Ensure these flushes happen prior to
bgfx::shutdown() so all deferred native destroys run.
| struct Buffer { | ||
| bgfx::VertexBufferHandle vbh = BGFX_INVALID_HANDLE; | ||
| bgfx::DynamicVertexBufferHandle dvbh = BGFX_INVALID_HANDLE; | ||
| bgfx::IndexBufferHandle ibh = BGFX_INVALID_HANDLE; | ||
| bgfx::DynamicIndexBufferHandle dibh; | ||
| bool is_dynamic = false; | ||
| bool is_index = false; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++, if a struct member like bgfx::DynamicIndexBufferHandle dibh; has no default member initializer, is that member indeterminate after default construction?
💡 Result:
Yes—if the member has no default member initializer and you default-initialize the enclosing object such that that member is not mentioned in a constructor initializer list, then for automatic storage duration the member’s value is indeterminate (i.e., it has unspecified/indeterminate contents), and reading it is undefined behavior except in limited cases. Reasoning (C++ initialization rules): 1) Default-initialization of an object with automatic (stack) or dynamic storage duration results in “indeterminate value”, and if no initialization is performed for the object, it retains that indeterminate value until it is replaced [1]. 2) When a class object is default-initialized (e.g., S s; for a type with an implicitly-defined default constructor), each non-static data member that is not explicitly initialized by the constructor is also default-initialized; only members with default member initializers are initialized from those initializers [2]. 3) Therefore, for a member like bgfx::DynamicIndexBufferHandle dibh; with no default member initializer, default construction of the containing struct/class leaves dibh indeterminate (for automatic/dynamic storage), unless the handle type’s default constructor gives it a deterministic value. How to ensure a deterministic “null/invalid handle” value: - Use value-initialization: S s{}; (or S s = {};). Value-initialization zero-initializes the object before default construction is applied, which makes scalar subobjects become zero [3][4]. - Or add a default member initializer for the handle member (recommended if bgfx::DynamicIndexBufferHandle has an appropriate invalid/zero state). Net: No default member initializer + default-initialization of an automatic object => the member is indeterminate after construction, unless the member’s type default constructor itself initializes it determinately [1][2].
Citations:
- 1: https://cppreference.dev/w/cpp/language/default_initialization
- 2: https://timsong-cpp.github.io/cppwp/std20/dcl.init
- 3: https://cppreference.dev/w/cpp/language/value_initialization
- 4: https://abseil.io/tips/146
Initialize Buffer::dibh to BGFX_INVALID_HANDLE
In engine/native/rendering/rhi/rhi.cppm (struct Buffer, lines 114-121), dibh lacks a default member initializer; with default construction it can be left indeterminate unless bgfx::DynamicIndexBufferHandle’s default constructor deterministically sets it, which can cause UB when later used for cleanup/update paths. Initialize it to the same invalid sentinel as the other handle members.
🐛 Proposed fix
struct Buffer {
bgfx::VertexBufferHandle vbh = BGFX_INVALID_HANDLE;
bgfx::DynamicVertexBufferHandle dvbh = BGFX_INVALID_HANDLE;
bgfx::IndexBufferHandle ibh = BGFX_INVALID_HANDLE;
- bgfx::DynamicIndexBufferHandle dibh;
+ bgfx::DynamicIndexBufferHandle dibh = BGFX_INVALID_HANDLE;
bool is_dynamic = false;
bool is_index = false;
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/rhi/rhi.cppm` around lines 114 - 121, The Buffer
struct's DynamicIndexBufferHandle member dibh is not initialized which can leave
it indeterminate; modify struct Buffer so dibh is default-initialized to
BGFX_INVALID_HANDLE (matching vbh, dvbh, ibh) to ensure safe default
construction and proper cleanup/update paths, i.e. update the declaration of
dibh inside struct Buffer to use BGFX_INVALID_HANDLE.
| FramebufferHandle create_framebuffer(u32 width, u32 height, TextureFormat format) { | ||
| // We set render target flags so it can be attached to a framebuffer object | ||
| u64 flags = BGFX_TEXTURE_RT | BGFX_SAMPLER_U_CLAMP | BGFX_SAMPLER_V_CLAMP; | ||
|
|
||
| bgfx::TextureFormat::Enum bgfx_format = bgfx::TextureFormat::RGBA8; | ||
|
|
||
| bgfx::TextureHandle th = bgfx::createTexture2D(static_cast<u16>(width), static_cast<u16>(height), false, 1, bgfx_format, flags); |
There was a problem hiding this comment.
Honor the requested framebuffer format.
format is part of the function contract, but this path always creates an RGBA8 texture. Any caller asking for a different render-target format will silently get the wrong backing texture. Please map TextureFormat to the corresponding bgfx format here instead of hardcoding RGBA8.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/native/rendering/rhi/texture.cpp` around lines 54 - 60,
create_framebuffer currently ignores the requested TextureFormat and always uses
bgfx::TextureFormat::RGBA8; change it to map the incoming TextureFormat to the
appropriate bgfx::TextureFormat::Enum before creating the texture. Update
create_framebuffer to convert TextureFormat (the enum parameter) into a
bgfx_format via a switch or small helper (e.g., mapTextureFormatToBgfx) covering
all supported formats and providing a sensible fallback (or assert/log) for
unsupported values, then pass that bgfx_format into bgfx::createTexture2D
instead of the hardcoded RGBA8.
2 participants
This is a draft of a clang-format document.
I want comments on anything that should be changed, these are just what I think might be good defaults. I will run it on the project before it gets merged.
Please go to https://clang.llvm.org/docs/ClangFormatStyleOptions.html for referencing each value. I tried to go in order but some at the top are ones I think are most important, but thinking back on it probably should have gone in order
Summary by CodeRabbit
Release Notes
.clang-formatconfiguration targeting C++23 compliance.