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.