Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit fallthrough #12

Open
r-barnes opened this issue Jul 31, 2024 · 4 comments
Open

Implicit fallthrough #12

r-barnes opened this issue Jul 31, 2024 · 4 comments

Comments

@r-barnes
Copy link

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]]?

@ghaerr
Copy link
Owner

ghaerr commented Aug 1, 2024

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
Copy link
Author

r-barnes commented Aug 6, 2024

@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.

@bugcrazy
Copy link

bugcrazy commented Nov 2, 2024

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/

@ghaerr
Copy link
Owner

ghaerr commented Nov 2, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants