Coding style - use of return in functions

One common coding style generates a lot of warnings with the two compilers that I use - the use of return in the middle of functions. With respect to the good work that has been done, I think that in a lot of cases the function would be clearer with one entry and one exit point.

Here’s an example which is repeated through some of the widgets:

    if(info->result != NULL) return LV_RES_OK;
    else return ancestor_signal(win, sign, param);
    return LV_RES_OK;

In the above example the logic could be refined

    if(info->result == NULL)
        return ancestor_signal(win, sign, param);
    return LV_RES_OK;

But in some cases a stack return variable would be nicer (there was one already in the example I used)

    lv_res_t res = LV_RES_OK;
    if(sign == LV_SIGNAL_GET_STYLE) {
        lv_get_style_info_t * info = param;
        info->result = lv_list_get_style(win, info->part);
        if(info->result == NULL)
            res = ancestor_signal(win, sign, param);
    }
    else { ...
    }
    return res;

The disadvantage is that the functions can get deeply nested with braced blocks. Also I would not advocate changing existing code due to the possibility of introducing bugs. But in the above example the logic change would make sense.

Both my compilers complain about this function, but I’m sure it will be cleaned up for the release (I’d change it but are not sure of the intent). My point is that perhaps some lint type testing would be good.

int16_t lv_bar_get_value(const lv_obj_t * bar)
{
    LV_ASSERT_OBJ(bar, LV_OBJX_NAME);

    lv_bar_ext_t * ext = lv_obj_get_ext_attr(bar);
    return ext->cur_value;
    return LV_BAR_GET_ANIM_VALUE(ext->cur_value, ext->cur_value_anim);
}

Here’s another example which I see sometimes - using returns in switch statements.

lv_color_t (*blend_fp)(lv_color_t, lv_color_t, lv_opa_t);
    switch (mode) {
    case LV_BLEND_MODE_ADDITIVE:
        blend_fp = color_blend_true_color_additive;
        break;
    case LV_BLEND_MODE_SUBTRACTIVE:
        blend_fp = color_blend_true_color_subtractive;
        break;
    default:
        LV_LOG_WARN("fill_blended: unsupported blend mode");
        return;
        break;
    }

My compiler doesn’t like the dead code around the break; I’d guess some compilers don’t like cases without a break.

The change below would keep compilers happy

    lv_color_t (*blend_fp)(lv_color_t, lv_color_t, lv_opa_t) = NULL;
    switch (mode) {
    case LV_BLEND_MODE_ADDITIVE:
        blend_fp = color_blend_true_color_additive;
        break;
    case LV_BLEND_MODE_SUBTRACTIVE:
        blend_fp = color_blend_true_color_subtractive;
        break;
    default:
	LV_LOG_WARN("fill_blended: unsupported blend mode");
        break;
    }
    if(blend_fp != NULL)
    {
         ...
    }

Again, I don’t really want to change the existing code but for new code it might be worth keeping in mind.

It’s really much cleaner.

lv_bar_get_value is clearly no intentional but it’s still there…

I agree, the code quality could be improved a lot in this regard. Unfortunately, GCC has no warning for unreachable code so I can’t spot issues on my own.

I was considering “one exit point” a lot but due to the introduced deep nesting (as you mentioned) I dropped it.

Can you offer a linter?

Can you please open an issue for this topic on GitHub?

Cppcheck is a functional lint type program http://cppcheck.net/
It complains a lot mostly about style (reducing scope of variables, duplicate assignments) but will find repeated lines, logic errors and other coding problems.

For dead code I find the Keil compiler finds the dead code where other compilers do not. It might be possible to use the code limited evaluation version and just look at the compiler warnings.

I will open an issue on github but unfortunately making code changes and pull requests is a pain. I end up making each change manually into the source using the web interface, and the chance for a mistake is very high.