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

ALL: Added FIXME to several fall through locations #1337

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mgerhardy
Contributor

mgerhardy commented Sep 14, 2018

No description provided.

@mgerhardy

This comment has been minimized.

Contributor

mgerhardy commented Sep 14, 2018

Not sure how you deal with this. Should these fall through situations at least be documented? Using // fall through fixes the warnings for clang and gcc.

@digitall

This comment has been minimized.

Member

digitall commented Sep 25, 2018

@mgerhardy : Yes, the fall through situations should be documented so it is clear if they are intentional or mistakes / bugs hence why GCC is warning. I would have preferred if GCC / other compilers used a pragma preprocessor definition or similar to surpress this warning, rather than parsing a comment, since it is likely more prone to issues and the exact formatting / phrasing since we preferred FALLTHROUGH or similar in the past similar to our FIXME comments.

Anyway, each existing case should be examined. If it is clear that a break has been omitted and this could provoke a bug, the break should be added. If the fall through is clearly intentional from the logic, it should be commented. The issue is where it is unclear. I would suggest that in those cases, the comment should be added to surpress the compiler warning, but a FIXME comment should be added noting that the engine developer should check this and either remove the FIXME and/or correct the fall through if a bug / unintentional.

I will take a look at the cases you have identified and see if they can be dealt with in this way.

@bonki

This comment has been minimized.

Member

bonki commented Oct 22, 2018

These definitely need to be examined on a case-by-case basis. I went through a lot of these warnings a couple of months ago and the ones which still are there I unfortunately couldn't easily confirm. While I marked most of them intentional I did find several non-intentional fall-throughs (i.e. bugs) so we need to be thorough here.

We did also agree on using // fall through which is what is used most right now in the tree. There are still (or maybe, again) exceptions to this but these should probably be changed to just that.

All in all, I personally am against merging this as-is because the compiler warnings are, in my book, enough to indicate that these still need looking into. That being said, the BLADERUNNER commit could be cherry-picked. AGI and MOHAWK can be verified by active devs (I think). With regard to PEGASUS, it is my understanding that we currently have no chance of verifying these ourselves given the state of the engine.

@digitall

This comment has been minimized.

Member

digitall commented Oct 22, 2018

Yes, I was not suggesting merging as-is ... but these cases can be looked at and dealt with, either by adding breaks when clear they are omitted accidently, marking when the fallthrough is intentional ... and then in the remaining cases, we should neuter the warning and add a FIXME for now.

@digitall

This comment has been minimized.

Member

digitall commented Nov 13, 2018

@mgerhardy : Thanks for indicating these. Have fixed them by various manual commits adding the required // fall through comments, but also a second comment with a FIXME so that engine author / developers can check and remove if testing / analysis shows if the fall through is functional / intentional.

Closing as fixed, but without merge.

@digitall digitall closed this Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment