ghaerr
Implicit fallthrough
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
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!
@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.
There are fixes in revision 140 and 141 for this issue:
https://sourceforge.net/p/agg/svn/140/
https://sourceforge.net/p/agg/svn/141/
@bugcrazy thanks for the information on revisions to the AGG SVN master for this issue as well as #13. It'd been about 4 years with few modifications on the SVN repo but now I see there's been several in 2024. I'll take a look at how all these might be added to this repo.
Thank you!