search

ghaerr / agg-2.6

AGG Anti-Grain Geometry Library
292 stars 80 forks source link

Implicit fallthrough #12

Open r-barnes opened 1 month ago

r-barnes commented 1 month ago

Compiling the code with clang's -Wimplicit-fallthrough reveals ~5 cases where fallthroughs are not explicitly marked (example). Are these bugs where break should have been used or are they intentional? If they are intentional, can they be marked with [[fallthrough]]?

https://github.com/ghaerr/agg-2.6/blob/c4f36b4432142f22c0bf82c6fbdb41567a236be2/agg-src/include/agg_conv_adaptor_vcgen.h#L102

ghaerr commented 1 month ago

Hello @r-barnes,

The example you show is definitely a fallthrough case, I haven't checked any others but it would want to made certain before adding an attribute, which probably requires a test suite. However, in general, this repository is attempting to be reference copy of AGG v2.6 with regards to AGG internals (except for serious bug fixes, since the master repo isn't on GitHub) so we probably don't want to add [[fallthrough]] to the repo. Adding attributes might also require C++11, right? I'm not certain what minimum version C++ is required for AGG.

Thank you!

r-barnes commented 1 month ago

@ghaerr - The [[fallthrough]] attribute was added in C++17, though __attribute__((fallthrough)) is available for earlier code. Most of the repositories I've worked with choose to use a preprocessor macro so that the fallthrough attribute is only added for compilers that support it (example).

In the much larger codebase that I found agg-2.6 integrated into we found quite a high bug rate associated with implicit fallthroughs (I'm working on getting the numbers on that), so enabling this protection was definitely a win for us. If you have internal code that this is interfacing with, enabling -Wimplicit-fallthrough for that code would probably be a quality/safety improvement.