bitmaptools.replace_color() function

[FoamyGuy]

New bitmaptools function to replace all pixels of a given color with a different color.

Most directly I want to use this inside of an "AccentableLabel" class that I am working on that allows certain passages of text within the label to use a different foreground/background color scheme from the rest of the text.

Having this replace_color() function makes that easy and quick because it can blit from the glyph bitmap and then replace the 1s with a different color from the palette afterward.

[dhalbert]

Nice addition!

Suggestions:

• Arg name change simplification.
• Validate arg.
• Reuse an existing error message.
• Remove superfluous default args.

[dhalbert]

Suggested change

	//| dest_bitmap: displayio.Bitmap, old_color: int, new_color: int
	//| ) -> None:
	//| """Replace any pixels of old_color with new_color in the dest_bitmap
	//|
	//| :param bitmap dest_bitmap: Destination bitmap that will be written into
	//| bitmap: displayio.Bitmap, old_color: int, new_color: int
	//| ) -> None:
	//| """Replace any pixels of ``old_color`` with ``new_color`` in the ``bitmap``
	//|
	//| :param displayio.Bitmap bitmap: bitmap that will be change

[FoamyGuy]

Thank you! I've made those changes in the latest commit.

I went ahead and removed {.u_obj = MP_OBJ_NULL} from other instances where it was used with required args in this file as well.

[dhalbert]

The ports/unix build is failing with the missing initializers, e.g.:

../../shared-bindings/bitmaptools/__init__.c:188:9: error: missing initializer for field ‘defval’ of ‘mp_arg_t’ {aka ‘const struct _mp_arg_t’} [-Werror=missing-field-initializers]
  188 |         {MP_QSTR_dest_bitmap, MP_ARG_REQUIRED | MP_ARG_OBJ},

This is because ports/unix compiles with -Wextra, which enables -Wmissing-field-initializers. The other ports do not do -Wextra, though there is mention of it in various pieces of library code.

There are missing initializers in quite a few places, such as in shared-bindings/busio/I2C.c. Butports/unix does not happen to encounter these because they were not included in the compilation, e.g. I2C.c is not included in ports/unix. However, bitmaptools is included in ports/unix.

The gcc documentation for -Wmissing-field-initializers is quoted below. As I read elsewhere, it's not warning that the initialization is missing (there is a default initialization to 0), it's that the initializers are missing.

-Wmissing-field-initializers

Warn if a structure’s initializer has some fields missing. For example, the following code causes such a warning, because x.h is implicitly zero:

struct s { int f, g, h; };
struct s x = { 3, 4 };

In C this option does not warn about designated initializers, so the following modification does not trigger a warning:

struct s { int f, g, h; };
struct s x = { .f = 3, .g = 4 };

In C this option does not warn about the universal zero initializer ‘{ 0 }’:

struct s { int f, g, h; };
struct s x = { 0 };

Likewise, in C++ this option does not warn about the empty { } initializer, for example:

struct s { int f, g, h; };
s x = { };

This warning is included in -Wextra. To get other -Wextra warnings without this one, use -Wextra -Wno-missing-field-initializers.

So I think we should just add -Wno-missing-field-initializers in ports/unix/Makefile, and leave off the initializers as I suggested in the review. That makes some error checking a tiny bit less robust. Alternatively we could used designated initializers in the arg initializers, but it's a lot of work to add those everywhere. I think it would also change some code we inherit from MicroPython.

[dhalbert]

(Adding -Wextra to a port build triggers other error'd warnings as well. For instance, on raspberrypi I saw some warnings about the status LED preprocessor macro definition:

 311 |     #if CIRCUITPY_STATUS_LED
      |         ^~~~~~~~~~~~~~~~~~~~
../../supervisor/shared/status_leds.c:311:9: warning: this use of "defined" may not be portable [-Wexpansion-to-defined]

)

[FoamyGuy]

I added -Wno-missing-field-initializers to the unix port Makefile in the latest commit and confirmed that it does allow it to build successfully locally.

[dhalbert]

Thanks for re-testing after the build issues.