Missing breaks

[BsAtHome]

There are two unresolved switch case fall-through warnings that need review.

Problem case 1:

The code in emc/rs274ngc/interp_o_word.cc seems to be executed as a result of a python callable O-word, where the return value is integer. When the return value is double (RET_DOUBLE), then the case is concluded with a break. However, when the return value is integer (RET_INT), then the case falls through to the default case. Code in question (lines 621-659):

int Interp::handler_returned( setup_pointer settings,  context_pointer active_frame, const char *name, bool osub) {
...
    switch (active_frame->pystuff.impl->py_return_type) {
...
    case RET_DOUBLE: 
	if (osub) { // float values are ok for osubs
	    settings->return_value = active_frame->pystuff.impl->py_returned_double;
	    settings->value_returned = 1;
	} else {
	    ERS("handler_returned: %s returned double: %f - invalid", 
			name, active_frame->pystuff.impl->py_returned_double);
	}
	break;

    case RET_INT: 
	if (osub) { // let's be liberal with types - widen to double return value
	    settings->return_value = (double) active_frame->pystuff.impl->py_returned_int;
	    settings->value_returned = 1;
	} else 
	    return active_frame->pystuff.impl->py_returned_int;

    case RET_ERRORMSG:
...

Not only is the break missing, but the else-clause within the RET_INT case is different from the RET_DOUBLE case. The double case calls (expands macro) ERS(), which stacks an error message and returns an INTERP_ERROR. The RET_INT case does not do the same thing.

I think the resolution should be to align the RET_INT case with the RET_DOUBLE case and change the RET_INT case to have both a break at the end and call ERS() in the else-clause.

Problem case 2:

Another part of the code in emc/rs274ngc/interp_o_word.cc handles O-word returns. There is a case in a sub-switch that handles INTERP_EXECUTE_FINISH, which falls through to the default case. Code in question (lines 392-514):

int Interp::execute_return(setup_pointer settings, context_pointer current_frame,int call_type) {
...
    switch (call_type) {

    case CT_REMAP:
	switch (settings->call_state) {
	case CS_NORMAL:
   	case CS_REEXEC_EPILOG:
...
		switch (status = handler_returned(settings, current_frame, current_frame->subName, false)) {
		case INTERP_EXECUTE_FINISH:
		    settings->call_state = CS_REEXEC_EPILOG;
		    eblock->call_type = CT_REMAP;
		    CHP(status);
		default:
		    settings->call_state = CS_NORMAL;
		    settings->sequence_number = previous_frame->sequence_number;
		    CHP(status);
		    // leave_context() is done by falling through into CT_NGC_OWORD_SUB code
		}
	    }
	}
	// fall through to normal NGC return handling 
...

Falling through from INTERP_EXECUTE_FINISH causes settings->calls_state to be overwritten, which seems the wrong thing to do.

I think there should be a break at the end of the INTERP_EXECUTE_FINISH case (after the CHP(status) macro).

Please advise on these two problem cases.

[BsAtHome]

As to problem 1:
Changing the else-clause is not correct. It makes a lot of the remap tests fail. Apparently, the integer returned is used.

However, I still think that there should be a break at the end of the RET_INT case. It would not makes sense to go through the trouble to setup the settings->... and then fall through to case RET_ERRORMSG and drop out with an error status.

All proposed changes from -Wimplicit-fallthrough can be seen here: master...BsAtHome:linuxcnc:refs/heads/fix_implicit-fallthrough

[rmu75]

Can anybody say if O-word numbers can indeed have a decimal point? I think I tried with G70 and at least there it is not accepted.

[andypugh]

Can anybody say if O-word numbers can indeed have a decimal point? I think I tried with G70 and at least there it is not accepted.

Without looking into this at all....
All numbers in G-code are floats at the point that they enter the interpreter. This handler may be distinguishing between O-words called from G-code (as a float) and by other means (with an Int)

[BsAtHome]

The RET_INT part is from return values through the python interface. It appears to come from remap, where you can map a python function to an O-word. That part seems to be understood.

The question is whether it should fall through to RET_ERRORMSG resulting in an error. That seems wrong if you look at the RET_DOUBLE case. Also, the comment seems to indicate that a python mapped O-word returning an int is simply promoted to double and all should be swell. Therefore, there should be a break at the end of the RET_INT case. That is the question.

[andypugh]

The first one definitely looks wrong, I can only assume that it is a seldom-used bit of code.
I will try to check out the obvious fix tomorrow.
The second also looks fairly blatantly wrong, as is sets settings->call_state to one thing, and then immediately to another.

[andypugh]

I decided that it almost certainly was as simple as it looked, and have pushed a fix.

[BsAtHome]

Ah yes, thanks.

However, those weren't the only missing breaks. I'll make a PR to cover the remaining few (which are obvious errors).