GRAPHICS: Add a function for creating maps to PaletteLookup #5408
Conversation
|
@coderabbitai review |
WalkthroughWalkthroughThe changes introduce a new function Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
graphics/palette.cpp
Outdated
| return bestColor; | ||
| } | ||
|
|
||
| uint32 *PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | ||
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len) == 0) | ||
| return nullptr; | ||
|
|
||
| uint32 *map = new uint32[len]; | ||
| for (uint i = 0; i < len; i++) { | ||
| byte r = *srcPalette++; | ||
| byte g = *srcPalette++; | ||
| byte b = *srcPalette++; | ||
|
|
||
| map[i] = findBestColor(r, g, b, useNaiveAlg); | ||
| } | ||
| return map; | ||
| } |
There was a problem hiding this comment.
The createMap function allocates memory for the map but does not provide a mechanism to free it. This could lead to memory leaks if the caller does not properly manage the returned pointer. Consider using smart pointers to manage memory automatically.
- uint32 *map = new uint32[len];
+ std::unique_ptr<uint32[]> map(new uint32[len]);And change the function return type to std::unique_ptr<uint32[]>.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| return bestColor; | |
| } | |
| uint32 *PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | |
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len) == 0) | |
| return nullptr; | |
| uint32 *map = new uint32[len]; | |
| for (uint i = 0; i < len; i++) { | |
| byte r = *srcPalette++; | |
| byte g = *srcPalette++; | |
| byte b = *srcPalette++; | |
| map[i] = findBestColor(r, g, b, useNaiveAlg); | |
| } | |
| return map; | |
| } | |
| return bestColor; | |
| } | |
| std::unique_ptr<uint32[]> PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | |
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len) == 0) | |
| return nullptr; | |
| std::unique_ptr<uint32[]> map(new uint32[len]); | |
| for (uint i = 0; i < len; i++) { | |
| byte r = *srcPalette++; | |
| byte g = *srcPalette++; | |
| byte b = *srcPalette++; | |
| map[i] = findBestColor(r, g, b, useNaiveAlg); | |
| } | |
| return map; | |
| } |
There was a problem hiding this comment.
@coderabbitai can you check the consideration points for this review to see if there is something we ask you not use in this code review?
There was a problem hiding this comment.
@neuromancer I apologize for any confusion, but as an AI, I don't have access to specific project guidelines or consideration points for this review beyond the information provided in the pull request and comments. If there are specific guidelines or restrictions on using certain features like smart pointers, could you please provide more details or point me to the relevant documentation? I'm here to help and adjust my suggestions based on your project's requirements.
There was a problem hiding this comment.
@coderabbitai Please consider the following project guidelines points:
- We do not use C++ exceptions
- We do not use the std C++ library in our codebase, therefore, we do not use STL classes in our codebase
- We have predefined common integer types. Use them instead of rolling your own: byte, int8, uint8, int16, uint16, int32, uint32
- All the input/output is performed using our own code which is abstracted in backends directory
Then rewrite or discard your current review.
There was a problem hiding this comment.
@neuromancer Thank you for providing the project guidelines. Given these constraints, my previous suggestion to use std::unique_ptr is not applicable as it's part of the STL.
Considering the guidelines, I suggest the following:
-
Document the ownership and responsibility of the allocated memory in the function's comments. Make it clear that the caller is responsible for freeing the memory when it's no longer needed.
-
If there's a common memory management strategy in your codebase, consider applying it here. For example, if you have a custom smart pointer implementation or a garbage collector, use that to manage the memory.
-
If possible, consider an alternative design where you don't need to return a raw pointer. For example, you could pass in a pre-allocated buffer as an argument to the function, which would then fill it with the map data. This way, the caller would be responsible for both allocation and deallocation, keeping the memory management in one place.
Please let me know if you need further assistance or if there's a specific approach you'd like to discuss.
| byte r = *srcPalette++; | ||
| byte g = *srcPalette++; | ||
| byte b = *srcPalette++; |
There was a problem hiding this comment.
The increment of srcPalette pointer is not checked against the length of the source palette. This could lead to reading beyond the end of the source palette if len is larger than the actual size of the source palette. Consider adding a check to prevent this.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| return bestColor; | ||
| } | ||
|
|
||
| uint32 *PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | ||
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len * 3) == 0) | ||
| return nullptr; | ||
|
|
||
| uint32 *map = new uint32[len]; | ||
| for (uint i = 0; i < len; i++) { | ||
| byte r = *srcPalette++; | ||
| byte g = *srcPalette++; | ||
| byte b = *srcPalette++; | ||
|
|
||
| map[i] = findBestColor(r, g, b, useNaiveAlg); | ||
| } | ||
| return map; | ||
| } |
There was a problem hiding this comment.
The createMap function allocates memory for map but does not provide a mechanism to free it. This could lead to memory leaks. Consider returning a smart pointer (like std::unique_ptr) instead of a raw pointer to ensure that the memory is automatically deallocated when it is no longer needed.
- uint32 *PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) {
+ std::unique_ptr<uint32[]> PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) {
...
- uint32 *map = new uint32[len];
+ std::unique_ptr<uint32[]> map(new uint32[len]);
...
- return map;
+ return std::move(map);Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| return bestColor; | |
| } | |
| uint32 *PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | |
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len * 3) == 0) | |
| return nullptr; | |
| uint32 *map = new uint32[len]; | |
| for (uint i = 0; i < len; i++) { | |
| byte r = *srcPalette++; | |
| byte g = *srcPalette++; | |
| byte b = *srcPalette++; | |
| map[i] = findBestColor(r, g, b, useNaiveAlg); | |
| } | |
| return map; | |
| } | |
| return bestColor; | |
| } | |
| std::unique_ptr<uint32[]> PaletteLookup::createMap(const byte *srcPalette, uint len, bool useNaiveAlg) { | |
| if (len <= _paletteSize && memcmp(_palette, srcPalette, len * 3) == 0) | |
| return nullptr; | |
| std::unique_ptr<uint32[]> map(new uint32[len]); | |
| for (uint i = 0; i < len; i++) { | |
| byte r = *srcPalette++; | |
| byte g = *srcPalette++; | |
| byte b = *srcPalette++; | |
| map[i] = findBestColor(r, g, b, useNaiveAlg); | |
| } | |
| return std::move(map); | |
| } |
|
Thank you! |
The resulting map can be passed to
Graphics::crossBlitMap(), which allows for faster conversion of images with different palettes.