Mir

Code review comment for lp:~andreas-pokorny/mir/pixel-format-utils

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

No major problems, but there are enough minor issues to justify a "Needs Fixing".

(1) The copyright year is now wrong :)

(2) Using the strongly typed MirPixelFormat, you don't have to check for invalid types. So:
100 + if (mir_pixel_format_invalid < format &&
101 + format < mir_pixel_formats)
102 + return 8;
103 + return 0;
could just be:
    return 8;
This is because it's already a precondition that "format" is a valid MirPixelFormat. The same goes for other functions. You never need to range-check a strongly typed enum.

(3) I assume the valid_format() function is for testing non-MirPixelFormat variables; integers? As such it might make sense for:
    bool valid_format(MirPixelFormat format)
to be:
    bool valid_mir_pixel_format(int format)
?

(4) TEST(MirPixelformat, ...
It is more correct and still safe to use the correct capitalization "MirPixelFormat". Although calling the test "PixelFormatUtils" might be even more accurate.

review: Needs Fixing

« Back to merge proposal