search

dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
631 stars 180 forks source link

Remove use of the `fallthrough` macro #252

Closed bjosv closed 1 year ago

bjosv commented 1 year ago

Keeping pattributes in the portable library in case the usercode requires it. The fallthrough define is no longer used internally and therefor not defined by default, i.e PORTABLE_EXPOSE_ATTRIBUTES is not enabled by default.

No implicit-fallthrough build errors given in ./scripts/test.sh

Fixes #247

bjosv commented 1 year ago

Maybe you liked the label names like lbl_fallthrough better?

mikkelfj commented 1 year ago

Well, on naming, in cases where fallthrough is at the very core, maybe fallthrough is the right label, but in general I tend to prefer names where it would also work if I moved the code around.

mikkelfj commented 1 year ago

I'm still looking through the code, but one thing: I think the label should be after the case label, not before. This both ensures that when moving code, it remains stable, and technically you still have fall through if you goto before the case label. And this is also how I have seen others suggest it.

mikkelfj commented 1 year ago

Yeah, aside from the two comments I added, I think it looks fine. Let me know if you disagree.

bjosv commented 1 year ago

I'll update, I've now seen some suggestion about having the label after the case. I was thinking that if a new case was entered in the middle, or if a case was moved it wouldn't be a fallthrough anymore, more of a stepping over a case..

mikkelfj commented 1 year ago

I was thinking that if a new case was entered in the middle, or if a case was moved it wouldn't be a fallthrough anymore, more of a stepping over a case..

True, speaking generally and on this code in particular, often fall through is just a convenient placement and not truly a fall through, more like implementing a flow graph. Rather than trying to emulate fall through I think it is better to see at as a goto driven flow graph similar to goto error: labels. But it depends on the situation. In the case with falling numbers, fall through is at the very core of the logic.

mikkelfj commented 1 year ago

OK if you are happy, I'm ready to merge.

mikkelfj commented 1 year ago

OK if you are happy, I'm ready to merge.

bjosv commented 1 year ago

It got a bit verboser, but I'm happy its according to recommendations.