Crusade Against Stupid Switch Statements: Part 0 of N - #1325
Crusade Against Stupid Switch Statements: Part 0 of N#1325VictorSohier wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds slice-backed IO primitives, block-based image readers and writers for uncompressed formats, CPU-dispatched format handlers, and a mipmap conversion path using ChangesImage IO conversion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ImageConvert
participant ImageReader
participant ImageWriter
participant SliceIO
ImageConvert->>ImageReader: create source mipmap reader
ImageReader->>SliceIO: read ColorRGBAF32x16 blocks
ImageConvert->>ImageWriter: create destination mipmap writer
ImageWriter->>SliceIO: write and flush blocks
ImageConvert->>ImageReader: destroy reader
ImageConvert->>ImageWriter: destroy writer
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
core/io/image.cpp (1)
536-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse distinct variables for source and destination mip layout.
mip_offset/mip_sizehold the source layout at Line 545, then are silently repurposed for the destination at Line 564, while the destination offset is fetched a third time at Line 548. Separate names make the two slices unambiguous and drop the redundant call. Also considerim_reader/im_writerto match the snake_case locals used elsewhere in this file.♻️ Suggested restructuring
- int64_t mip_offset = 0; - int64_t mip_size = 0; + int64_t src_offset = 0; + int64_t src_size = 0; + int64_t dst_offset = 0; + int64_t dst_size = 0; int mip_width = 0; int mip_height = 0; IO::Reader reader = {}; IO::Writer writer = {}; - IO::Image::Reader imReader = {}; - IO::Image::Writer imWriter = {}; + IO::Image::Reader im_reader = {}; + IO::Image::Writer im_writer = {}; ColorRGBAF32x16 block; - get_mipmap_offset_size_and_dimensions(mip, mip_offset, mip_size, mip_width, mip_height); + get_mipmap_offset_size_and_dimensions(mip, src_offset, src_size, mip_width, mip_height); + new_img.get_mipmap_offset_and_size(mip, dst_offset, dst_size); - const uint8_t *rptr = data.ptr() + mip_offset; - uint8_t *wptr = new_img.data.ptrw() + new_img.get_mipmap_offset(mip); + const uint8_t *rptr = data.ptr() + src_offset; + uint8_t *wptr = new_img.data.ptrw() + dst_offset;🤖 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 `@core/io/image.cpp` around lines 536 - 548, Update the mipmap processing block around get_mipmap_offset_size_and_dimensions to keep separate source and destination offset/size variables, avoiding reuse of mip_offset/mip_size and the redundant destination offset lookup. Rename imReader and imWriter to im_reader and im_writer to match the surrounding local naming convention.
🤖 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 `@core/io/image_rw.cpp`:
- Around line 148-150: Update the L8 alpha initialization loop in the image
decoding path to assign opaque alpha values of 1.0f to all 16 lanes instead of
zero, preserving the grayscale RGB values while ensuring L8-to-RGBA conversions
are fully opaque.
- Around line 456-463: Update the indexing expression in the nested block/row
loops to compute a pixel index first, then multiply that index by
COMPONENT_COUNT[format] and add c before passing it to sliceAt. Apply the
component stride to both the block and row contributions while preserving the
existing bounds check and color lookup.
- Around line 335-342: The multi-component image readers omit the channel offset
and therefore read every channel from component zero. In core/io/image_rw.cpp
lines 335-342, update the byte-component index in the nested block-read loops to
include c; make the corresponding typed-component index change at lines 417-424.
Preserve the existing pixel and block indexing while applying the channel offset
at both sites.
- Around line 207-225: Update the LA8 read logic in core/io/image_rw.cpp lines
207-225 and the corresponding write logic in lines 251-259: calculate each
pixel’s complete block-and-row index first, then apply the two-byte stride to
the entire prefix before selecting luminance or alpha. Ensure both block and row
prefixes are multiplied by two for all LA8 component accesses.
- Around line 168-172: In the L8 grayscale conversion at core/io/image_rw.cpp
lines 168-172, multiply each normalized RGB channel by 255.0f before converting
to uint64_t, preserving tonal detail while retaining the existing weighted
conversion. Apply the same pre-cast scaling in the LA8 conversion at
core/io/image_rw.cpp lines 251-255; both sites require direct changes.
- Around line 120-127: Update the block-reading and conversion paths in
core/io/image_rw.cpp at lines 120-127, 193-200, 324-331, 406-413, and 487-494
for the L8, LA8, RGBA8-family, float/half, and RGBA4444 readers. Track the
number of valid rows in the final block when the image height is not divisible
by four, avoid treating the short slice read as EOF, and ensure each
corresponding writer flushes only those valid rows rather than all four.
- Around line 1002-1005: Update the reader initialization path around
READER_STATE_CONSTRUCTOR so that, after successful construction,
dst->state.currentBlock is initialized to ceil(width / 4) rather than zero.
Preserve the existing zero-based initialization required by writer state, and
only apply the change to successfully constructed readers.
- Around line 95-100: Update the allocation in the image-state creation path
around UncompressedImageState to use Memory::alloc_aligned_static (or equivalent
64-byte aligned allocation) instead of malloc, matching blocks4x4 alignment.
Update the corresponding destroy/deallocation path to recover and apply the
stored allocation offset before freeing the original allocation.
- Around line 990-997: Guard the CPU feature dispatch in core/io/image_rw.cpp
lines 990-997 and 1015-1022 with architecture/compiler checks so
__builtin_cpu_supports for AVX2, AVX, and SSE4.2 is compiled only on supported
x86/GNU targets. On all other targets, have both reader and writer dispatch
paths use the scalar function tables.
In `@core/io/image.cpp`:
- Around line 565-582: Update the image processing flow containing
IO::Writer::make and IO::Image::Writer::make so every construction failure
routes through a shared cleanup path instead of returning immediately. Ensure
that path destroys reader, imReader, writer, and imWriter as appropriate,
reports the IO error with ERR_FAIL_MSG, and preserves normal teardown on
success.
- Around line 555-563: Unify the per-mip failure handling around the
IO::Image::Reader::make and related construction calls in
core/io/image.cpp:555-563 and core/io/image.cpp:565-582 by replacing the break
and bare returns with one cleanup/abort path. Ensure it destroys every object
constructed up to each failure, reports the error through ERR_FAIL_MSG, returns
before _copy_internals_from(new_img), and never publishes new_img; both
consolidated sites require these changes.
- Around line 583-596: Update the block read/write flow around the do/while loop
and Writer::flush so the loop’s terminal error is saved before flushing. Use the
flush result only when the loop completed with IO::Error::Okay, preserve non-Eof
loop failures, and ensure compressed formats using the block reader return the
loop failure—including Eof—instead of publishing a truncated image.
In `@core/io/io.cpp`:
- Around line 49-55: Update the read logic around Slice::copy so it copies from
state->buffer starting at state->offset into only the first count bytes of
buffer. Subslice both the source and destination before copying, then retain the
existing state->offset advancement and EOF handling.
In `@core/templates/Slice.cpp`:
- Around line 38-46: Correct the overlap-direction logic in Slice::copy so
copies with dst.data greater than src.data and overlapping ranges use backward
iteration, while forward iteration handles dst before src and non-overlapping
ranges. Preserve copying only MIN(src.length, dst.length) bytes and validate
behavior against memmove-equivalent overlap cases; alternatively delegate to
memmove.
In `@core/templates/Slice.h`:
- Around line 50-54: Update the bounds validation used by Slice::subslice and
Slice::set so ranges ending exactly at src.length are accepted, while still
rejecting ranges that exceed the source or overflow. Ensure defaultFlush and
sliceWrite receive valid end-reaching subslices and preserve their existing copy
behavior.
- Around line 45-49: Update the bounds check in Slice::get and the corresponding
Slice::set access path to allow an element whose end offset exactly equals
self.length, while still rejecting accesses beyond the slice. Preserve the
existing null/invalid behavior for out-of-bounds indices and ensure
final-element access works for both sliceAt callers and Slice::set.
---
Nitpick comments:
In `@core/io/image.cpp`:
- Around line 536-548: Update the mipmap processing block around
get_mipmap_offset_size_and_dimensions to keep separate source and destination
offset/size variables, avoiding reuse of mip_offset/mip_size and the redundant
destination offset lookup. Rename imReader and imWriter to im_reader and
im_writer to match the surrounding local naming convention.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bd414bc-20ad-4717-a604-f1bfd4beadb5
📒 Files selected for processing (8)
core/io/image.cppcore/io/image.hcore/io/image_rw.cppcore/io/image_rw.hcore/io/io.cppcore/io/io.hcore/templates/Slice.cppcore/templates/Slice.h
| void Slice::copy(Slice dst, Slice src) noexcept { | ||
| uintptr_t s = (uintptr_t)src.data; | ||
| uintptr_t d = (uintptr_t)dst.data; | ||
| size_t n = MIN(src.length, dst.length); | ||
| size_t i; | ||
| bool srcTailDoesntOverlapDstHead = s <= (d - n); | ||
| const uint8_t *u8src = (const uint8_t *)src.data; | ||
| uint8_t *u8dst = (uint8_t *)dst.data; | ||
| if (srcTailDoesntOverlapDstHead) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Overlap-direction check in Slice::copy is inverted for the case it actually needs to handle — corrupts overlapping copies with dst ahead of src.
srcTailDoesntOverlapDstHead = s <= (d - n) is true only when src lies entirely below dst with no overlap (d >= s+n); in that branch the code does a backward copy (unnecessary but harmless, since there's no overlap). The one case that genuinely requires backward iteration to avoid corruption — dst overlapping src with dst > src (e.g. shifting data to higher addresses within the same buffer) — instead falls into the else (forward) branch, because d - n < s there, making the check false.
Concrete trace: s=1000, d=1005, n=10 (overlap, d>s). Forward copy writes dst[1005]=src[1000] first, which overwrites the original value at address 1005; later, dst[1010]=src[1005] then reads back the already-clobbered value instead of the true original — a real data-corruption case for a primitive explicitly intended to be "overlap-safe."
🐛 Proposed fix (swap which branch does backward vs forward)
- bool srcTailDoesntOverlapDstHead = s <= (d - n);
+ bool needsBackwardCopy = (d > s) && (d < (s + n));
const uint8_t *u8src = (const uint8_t *)src.data;
uint8_t *u8dst = (uint8_t *)dst.data;
- if (srcTailDoesntOverlapDstHead) {
+ if (needsBackwardCopy) {
i = n;
while (i > 0) {Given the risk of subtly mis-deriving pointer-overlap arithmetic by hand, consider validating any fix against std::memmove-equivalent test vectors (overlap in both directions, and non-overlapping) before merging — or simply delegate to memmove(), which the standard library already implements correctly and portably.
Also applies to: 108-109
🤖 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 `@core/templates/Slice.cpp` around lines 38 - 46, Correct the overlap-direction
logic in Slice::copy so copies with dst.data greater than src.data and
overlapping ranges use backward iteration, while forward iteration handles dst
before src and non-overlapping ranges. Preserve copying only MIN(src.length,
dst.length) bytes and validate behavior against memmove-equivalent overlap
cases; alternatively delegate to memmove.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/core/templates/test_slice.h (2)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeader name breaks the engine's snake_case file convention.
Everything else under
core/templates/is lowercase (hash_map.h,local_vector.h, …).Slice.h/Slice.cppwill also bite on case-insensitive filesystems if anyone later writesslice.h. Worth renaming tocore/templates/slice.hwhile the surface area is still small.🤖 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 `@tests/core/templates/test_slice.h` at line 35, Rename the Slice header and corresponding implementation file to lowercase slice.h and slice.cpp, then update all references, including the include in test_slice.h and any related build or source references, to use the new names consistently.
97-116: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an unaligned-base case to
set byte.
value[64]is naturally aligned, and every subslice starts at offset 0, so the alignment prologue inSlice::set(and its 8-byte store path) is never exercised on a misaligned base — which is exactly why the&vs&&defect incore/templates/Slice.cppLine 43 slips through. Iterating the start offset as well would cover it.💚 Suggested addition
for (i = 0; i < 64; i += 1) { Slice::subslice(&sub, slice, 0, i); Slice::set(sub, i); ... } + // Misaligned starts. + for (size_t off = 1; off < 8; off += 1) { + Slice::subslice(&sub, slice, off, 64 - off); + Slice::set(sub, 0xA5); + for (j = off; j < 64; j += 1) { + CHECK_EQ(*sliceAt(slice, uint8_t, j), 0xA5); + } + }🤖 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 `@tests/core/templates/test_slice.h` around lines 97 - 116, Extend the “[Slice] set byte” test to iterate over nonzero start offsets and construct each Slice from the corresponding unaligned address, while varying the subslice length as before. Validate only the affected range and untouched bytes for every start offset so Slice::set’s alignment prologue and 8-byte store path are exercised on misaligned bases.core/templates/Slice.cpp (1)
47-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the whole hand-rolled fill with
memset.The fallthrough ladder is correct for all remainders (1–7, 8–15, 16–23, 24–31, ≥32 verified), but it duplicates
memset, relies onget(dst, i, 1)whose bounds check only validates 1 byte while 2/4/8 bytes are stored, and castsvoid*to wider integer types (alignment + aliasing exposure).memset(dst.data, n, dst.length)is vectorized by every mainstream libc and removes both the alignment prologue and this switch, along with then16/n32/n64locals.♻️ Suggested simplification
void Slice::set(Slice dst, uint8_t n) noexcept { - uint16_t n16 = 0x0101 * n; - ... + if (dst.data && dst.length) { + memset(dst.data, n, dst.length); + } }🤖 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 `@core/templates/Slice.cpp` around lines 47 - 106, Replace the hand-rolled fill loop and its n16/n32/n64 setup with a single memset call over dst.data for dst.length bytes using the byte value n. Remove the switch, get-based wide stores, and now-unused intermediate values while preserving the existing fill behavior.
🤖 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 `@core/io/image_rw.cpp`:
- Around line 622-638: Update the staging-buffer refill logic in the image read
routine to refresh the local row value after incrementing state->currentRow,
alongside the existing block refresh. Ensure subsequent bounds checks and
decoding use the new row group rather than the previously captured row.
- Around line 672-677: Update the Slice::subslice call in the image-writing loop
to pass a byte count consistently: compute the row pixel count as
MIN(state->currentBlock << 2, state->res[0]), then multiply it by
CONSTANT_FACTORS[state->format] for the subslice length while preserving the
existing byte-based offset.
- Around line 668-670: Update the error branch in the flush loop to preserve the
existing non-Okay value of err when a seek fails, then exit the loop without
converting it to IO::Error::Okay. Keep the successful path unchanged so callers
can correctly report the flush failure.
- Around line 640-646: Update the pixel decode in the loop around sliceAt so
RGBA4444 indexing uses the current pixel and component: remove the duplicated
block offset, decode the pixel’s two-byte value once, select the appropriate
byte and 4-bit nibble using i.c, and normalize the extracted nibble to the 0..1
range before assigning colors->c[i.c][i.p].
- Around line 223-226: Clamp float and half channel values to the 0..1 range
before UNORM quantization: update the weighted L8 conversion at
core/io/image_rw.cpp lines 223-226, the luminance and alpha conversions at lines
306-310, and every packed 8-bit component before multiplying by 255 at line 436.
Preserve the existing quantization formulas after clamping each source
component.
- Around line 188-190: Update the L8 sample assignments in the image-reading
block so the uint8 value is normalized to the 0..1 range before storing it in
each float color channel. Apply the same normalized sample consistently to
colors->r, colors->g, and colors->b, preserving the existing indexing and
channel replication.
In `@core/templates/Slice.cpp`:
- Around line 42-46: Fix the alignment loop in Slice.cpp to use logical
short-circuiting: check i < dst.length first, then continue while the
destination address plus i remains 8-byte misaligned. Preserve the byte-wise
initialization and ensure the loop exits on alignment or exhaustion before the
subsequent wide stores.
---
Nitpick comments:
In `@core/templates/Slice.cpp`:
- Around line 47-106: Replace the hand-rolled fill loop and its n16/n32/n64
setup with a single memset call over dst.data for dst.length bytes using the
byte value n. Remove the switch, get-based wide stores, and now-unused
intermediate values while preserving the existing fill behavior.
In `@tests/core/templates/test_slice.h`:
- Line 35: Rename the Slice header and corresponding implementation file to
lowercase slice.h and slice.cpp, then update all references, including the
include in test_slice.h and any related build or source references, to use the
new names consistently.
- Around line 97-116: Extend the “[Slice] set byte” test to iterate over nonzero
start offsets and construct each Slice from the corresponding unaligned address,
while varying the subslice length as before. Validate only the affected range
and untouched bytes for every start offset so Slice::set’s alignment prologue
and 8-byte store path are exercised on misaligned bases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 52f47c68-3f36-4a62-8f9c-af9b331b8d5e
📒 Files selected for processing (9)
core/io/image.cppcore/io/image_rw.cppcore/io/image_rw.hcore/io/io.cppcore/io/io.hcore/templates/Slice.cppcore/templates/Slice.htests/core/templates/test_slice.htests/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- core/io/image_rw.h
- core/templates/Slice.h
- core/io/io.h
- core/io/io.cpp
- core/io/image.cpp
| (*sliceAt(data, uint8_t, (i.row * width) + (block << 2) + b)) = (uint8_t)(((13938U * ((uint64_t)(colors->r[i.p] * 255.0))) + | ||
| (46869U * ((uint64_t)(colors->g[i.p] * 255.0))) + | ||
| (4729U * ((uint64_t)(colors->b[i.p] * 255.0))) + 32768U) >> | ||
| 16); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp float channels before UNORM quantization. Float and half source formats can contain negative or over-one values. These casts wrap or invoke invalid float-to-integer conversion behavior instead of producing saturated byte output.
core/io/image_rw.cpp#L223-L226: clamp weighted L8 source channels to0..1before quantizing.core/io/image_rw.cpp#L306-L310: clamp LA8 luminance inputs and alpha before quantizing.core/io/image_rw.cpp#L436-L436: clamp every packed 8-bit component before multiplying by255.
📍 Affects 1 file
core/io/image_rw.cpp#L223-L226(this comment)core/io/image_rw.cpp#L306-L310core/io/image_rw.cpp#L436-L436
🤖 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 `@core/io/image_rw.cpp` around lines 223 - 226, Clamp float and half channel
values to the 0..1 range before UNORM quantization: update the weighted L8
conversion at core/io/image_rw.cpp lines 223-226, the luminance and alpha
conversions at lines 306-310, and every packed 8-bit component before
multiplying by 255 at line 436. Preserve the existing quantization formulas
after clamping each source component.
There was a problem hiding this comment.
This PR does not set out to fix existing bugs, merely to make the existing code more maintainable. If this ever becomes a problem, it will be fixed later. There is even a assertion in the tests for the one edge case where this may become a problem and it passes.
There was a problem hiding this comment.
@VictorSohier, understood. Since this refactor is intended to preserve existing behavior and the relevant edge case is already covered by a passing assertion, I’ll treat the clamping change as out of scope for this PR.
Would you like me to open a follow-up issue for saturated UNORM quantization, or leave it for a future report if it becomes relevant?
🐇💭
There was a problem hiding this comment.
| Slice::subslice( | ||
| &tmp, | ||
| state->data, | ||
| i * (state->res[0]) * CONSTANT_FACTORS[state->format], | ||
| MIN((state->currentBlock * CONSTANT_FACTORS[state->format]) << 2, state->res[0])); | ||
| err = IO::Writer::write(writer, nullptr, tmp); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass a byte count to subslice.
Line 676 mixes pixels and bytes. Compute row_pixels = MIN(currentBlock << 2, res[0]), then pass row_pixels * CONSTANT_FACTORS[format] as the subslice length.
🤖 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 `@core/io/image_rw.cpp` around lines 672 - 677, Update the Slice::subslice call
in the image-writing loop to pass a byte count consistently: compute the row
pixel count as MIN(state->currentBlock << 2, state->res[0]), then multiply it by
CONSTANT_FACTORS[state->format] for the subslice length while preserving the
existing byte-based offset.
| return true; | ||
| } | ||
|
|
||
| #define sliceAt(slice, t, i) ((t *)Slice::get(slice, i, sizeof(t))) |
There was a problem hiding this comment.
Why can´t these methods above just be be templates on element type instead of relying on macros like this?
template <typename T>
constexpr static void *get(
Slice self, size_t index) noexcept
{
void *ret = nullptr;
if ((sizeof(T) * (index + 1)) <= self.length) {
ret = static_cast<uint8_t*>(self.data) + (sizeof(T) * index);
}
return ret;
}
You can keep it based on void*
There was a problem hiding this comment.
I'd rather just have such a function wrap the void * version. I want to have the function be able to be compiled independently of the template. The important bit is that the internals are type erased.
2 participants
Found a stupid switch statement with some stupid templates and thought why not make this N^2 problem a 2N problem. It is currently at the point where it is a little more than a superset of the original switch statement. There are some placeholders for additional conversion routines and SIMD options with fallbacks to scalar implementations. This also contains some additional generic machinery for future IO tasks.
Summary by CodeRabbit