Operating System: All
Platform: All

On looking at this code more closely a 2nd time after a good night's sleep, I can actually now see that the if/s in the case sort of cancel each other out..

It's very confusing / non-obvious / hard to read code - but probably not really a bug then, as it looks like a switch case fall through bug, but because of the nested if actually is not (which I didn't notice yesterday).

This really should be refactored more clearly, under this bug.

As https://git.opendaylight.org/gerrit/#/c/53631/ and https://git.opendaylight.org/gerrit/#/c/53875/ are just about to add more tests for this code (according to private email from Faseela), perhaps someone could have a go at cleaning up the switch in IfmUtil.getEgressActionInfosForInterface() after those two changes for extended test coverage have been merged.

> * replaced with a break;

meanwhile had a chat with Faseela on IRC to confirm it's not a bug, it's supposed to be like this, it just confused us.

> * or documented.

just noticed on http://checkstyle.sourceforge.net/config_coding.html#FallThrough that CS has a something that's a bit nicer for this case then the SuppressWarnings, apparently if you put "// falls through" just before the case: without a "break;" that indicates to it that it's intentional and OK.

> nicer for this case then the @SuppressWarnings("checkstyle:FallThrough")

and, apparently @SuppressWarnings("fallthrough") helps javac understand that we really mean to do that, so we probably should put that as hit to javac.

(In reply to Michael Vorburger from comment #3)
> > nicer for this case then the @SuppressWarnings("checkstyle:FallThrough")
>
> and, apparently @SuppressWarnings("fallthrough") helps javac understand that
> we really mean to do that, so we probably should put that as hit to javac.

I followed your recommendation to comment the "fall-through":

https://git.opendaylight.org/gerrit/#/c/53896/5

Considering this resolved, as we've confirmed verbally with Faseela that the switch case "fall through" in IfmUtil.getEgressActionInfosForInterface() is intentional and not a bug, and "documented" this (not the "why" just the "fact that") via inline "// fall through", which Checkstyle detects and then shuts up about.

I did actually spend a moment to try to refactor it differently, but figured that the code becomes less not more readable - so let's just leave that as it is.

PS: We ARE hoping that the nascent but extremely minimalistic IfmUtilTest can be expanded upon to cover the IfmUtil.getEgressActionInfosForInterface() & more code better!

> nascent but extremely minimalistic IfmUtilTest

sorry, that was the wrong class of course; the InterfaceManagerConfigurationTest is actually one of our shining examples of Component Tests ... I've just quickly locally verified with http://www.eclemma.org, and it does seem to cover the case of VLAN_INTERFACE in IfmUtil.getEgressActionInfosForInterface() .. perhaps future work could cover the other cases (MPLS_OVER_GRE, GRE_TRUNK_INTERFACE, VXLAN_TRUNK_INTERFACE).