Miscellaneous refactoring #12
Conversation
| data = ctxmalloc(context, sizeof *data * context -> source -> width * context -> source -> height); | ||
| plum_convert_colors(data, context -> source -> data, (size_t) context -> source -> width * context -> source -> height, | ||
| PLUM_COLOR_32, context -> source -> color_format); | ||
| size_t size = (size_t) context -> source -> width * context -> source -> height; |
There was a problem hiding this comment.
Factoring out a common (size_t) context -> source -> width * context -> source -> height expression, as is done almost everywhere else.
| } | ||
| size_t rowsize = (size_t) context -> source -> width * 3, padding = 0; | ||
| if (rowsize & 3) { | ||
| padding = 4 - (rowsize & 3); | ||
| rowsize += padding; | ||
| } | ||
| unsigned char * out = append_output_node(context, rowsize * context -> source -> height); | ||
| unsigned char * output = append_output_node(context, rowsize * context -> source -> height); |
There was a problem hiding this comment.
output is a much more common name for this kind of variable elsewhere in the codebase.
| @@ -21,6 +21,26 @@ struct plum_rectangle * get_frame_boundaries (struct context * context, bool anc | |||
|
|
|||
| void adjust_frame_boundaries (const struct plum_image * image, struct plum_rectangle * restrict boundaries) { | |||
| uint64_t empty_color = get_empty_color(image); | |||
| #define checkframe(type) do \ | |||
There was a problem hiding this comment.
Here the palette code was so similar to the 16/32/64 color code that I wanted to use the existing checkframe macro for the palette as well.
| index += (size_t) boundaries[frame].height * image -> width; \ | ||
| for (size_t remaining = (size_t) (image -> height - boundaries[frame].top - boundaries[frame].height) * image -> width; remaining; remaining --) \ | ||
| if (notempty(type)) goto done ## type; \ | ||
| continue; \ |
There was a problem hiding this comment.
This continue is an alternative to a manually-checked bool adjust flag.
This control flow:
for (...) {
bool adjust = true;
if (various stuff) goto done;
adjust = false;
done:
if (adjust) finish up;
}was refactored to this:
for (...) {
if (various stuff) goto done;
continue;
done:
finish up;
}
| } \ | ||
| while (false) | ||
| #undef notempty | ||
| #define notempty(bits) (image -> data ## bits[index ++] != empty_color) |
There was a problem hiding this comment.
rgbds has already been converted to C++; next up, libplum, with templates and anonymous closures replacing all these macros. >:3
| @@ -12,15 +12,58 @@ void generate_GIF_data (struct context * context) { | |||
| if ((uint8_t) (depth >> 8) > overall) overall = depth >> 8; | |||
| if ((uint8_t) (depth >> 16) > overall) overall = depth >> 16; | |||
| if (overall > 8) overall = 8; | |||
| header[4] = (overall - 1) << 4; | |||
| header[5] = header[6] = 0; | |||
| bytewrite(header + 4, (overall - 1) << 4, 0, 0); | |||
There was a problem hiding this comment.
Using bytewrite for more than two values, same as elsewhere.
| if (context -> source -> palette) | ||
| generate_GIF_data_with_palette(context, header); | ||
| else | ||
| generate_GIF_data_from_raw(context, header); | ||
| byteoutput(context, 0x3b); | ||
| } | ||
|
|
||
| #define checkboundaries(buffertype, buffer, frame) do { \ |
There was a problem hiding this comment.
The generate_GIF_data_with_palette and generate_GIF_frame_data cases were very similar, so I factored them out into this macro.
(I'm sure there are even more such cases, but I found the ones that I did in this PR by inspecting the remaining gotos again. It's probably not a coincidence that the most complex goto logic is mostly found inside this sort of repetitive code.) (Non-complex gotos are the ones used for skipping to a failure case, or breaking out of nested loops/switches.)
| top = boundaries[frame].top; \ | ||
| width = boundaries[frame].width; \ | ||
| height = boundaries[frame].height; \ | ||
| } else { \ |
There was a problem hiding this comment.
This else takes the place of an unnecessary goto.
This control flow:
if (boundaries) {
if (various stuff) goto findbounds;
we have the bounds;
goto gotbounds;
}
findbounds:
more stuff;
gotbounds:was refactored to this:
if (boundaries) {
if (various stuff) goto findbounds;
we have the bounds;
} else {
findbounds:
more stuff;
}
| } \ | ||
| } \ | ||
| if (left || width != context -> source -> width) { \ | ||
| buffertype * target = buffer; \ |
There was a problem hiding this comment.
There'd be no need for buffertype in C23, just do typeof(buffer) target = buffer;.
| @@ -46,7 +46,7 @@ static inline uint16_t bitextend16 (uint16_t value, unsigned width) { | |||
| static inline void * append_output_node (struct context * context, size_t size) { | |||
| struct data_node * node = ctxmalloc(context, sizeof *node + size); | |||
| *node = (struct data_node) {.size = size, .previous = context -> output, .next = NULL}; | |||
| if (context -> output) context -> output -> next = node; | |||
| if (node -> previous) node -> previous -> next = node; | |||
There was a problem hiding this comment.
I think this is more appropriate, because it expresses the general case of replacing a doubly-linked list node, without hard-coding the thing that this one's previous node happens to be. Also it matches the resize_output_node function in the QOI PR.
| @@ -109,7 +109,7 @@ size_t compute_uncompressed_PNG_block_size (const unsigned char * restrict data, | |||
| if (length) { | |||
| score += length - 1; | |||
| if (score >= 16) break; | |||
| } else if (score > 0) | |||
| } else if (score) | |||
There was a problem hiding this comment.
score is unsigned, and everywhere else you avoid doing unsigned > 0.
| header[8] = (type < 4) ? 1 << type : (8 << (type >= 6)); | ||
| header[9] = (type >= 4) ? 2 + 4 * (type & 1) : 3; | ||
| bytewrite(header + 10, 0, 0, 0); | ||
| bytewrite(header + 8, (type < 4) ? 1 << type : (8 << (type >= 6)), (type >= 4) ? 2 + 4 * (type & 1) : 3, 0, 0, 0); |
There was a problem hiding this comment.
Using bytewrite for more than two values.
| for (unsigned char * out = buffer; node; node = node -> next) { | ||
| memcpy(out, node -> data, node -> size); | ||
| out += node -> size; | ||
| for (unsigned char * output = buffer; node; node = node -> next) { |
There was a problem hiding this comment.
The only other out variable is a uint_fast32_t in bmpwrite.c, not any pointers to output buffers.
| } | ||
| } | ||
|
|
||
| size_t get_total_output_size (struct context * context) { | ||
| size_t result = 0; | ||
| size_t output_size = 0; |
There was a problem hiding this comment.
Same name it's assigned to above.
| } else { | ||
| const uint32_t * sp = source; | ||
| convert(sp); | ||
| #define convert(frombits, tobits) do { \ |
There was a problem hiding this comment.
You already had the formatpair macro below, may as well use it here and save a couple of lines + make the to-from paired logic clearer.
Found some things while implementing QOI.