Tidying up the Library coding styles

[TerryE]

I've noticed that the current libraries haven't been cut by someone who is familiar with the Lua C interface APIs and as a result the only a subset of the API is used with standard features being reimplemented inline in the library code. A good example is in the gpio.c module where:

  const char *str = luaL_checklstring( L, 2, &sl );
  if (str == NULL)
    return luaL_error( L, "wrong arg type" );

  if(sl == 2 && c_strcmp(str, "up") == 0){
    type = GPIO_PIN_INTR_POSEDGE;
  }else if(sl == 4 && c_strcmp(str, "down") == 0){
    type = GPIO_PIN_INTR_NEGEDGE;
  }else if(sl == 4 && c_strcmp(str, "both") == 0){
    type = GPIO_PIN_INTR_ANYEDGE;
  }else if(sl == 3 && c_strcmp(str, "low") == 0){
    type = GPIO_PIN_INTR_LOLEVEL;
  }else if(sl == 4 && c_strcmp(str, "high") == 0){
    type = GPIO_PIN_INTR_HILEVEL;
  }else{
    type = GPIO_PIN_INTR_DISABLE;
  }

can be better coded:

  static const char * const opts[] = {"", "up", "down", "both", "low", "high"};
  static const int opts_type[] = { 
    GPIO_PIN_INTR_DISABLE, GPIO_PIN_INTR_POSEDGE, GPIO_PIN_INTR_NEGEDGE,
    GPIO_PIN_INTR_ANYEDGE, GPIO_PIN_INTR_LOLEVEL, GPIO_PIN_INTR_HILEVEL
    };
  int type = opts_type[luaL_checkoption(L, 2, "", opts)];

This is shorter, easier to read and uses less code space.

My suggestion is that it is not worth going though the modules tidying up such loose coding as a separate exercise, but that if anyone is doing material changes to any specific module, then it does make sense doing this tidy up also at the same time if there is a code space saving.

Though given Nick's recent comments, maybe this tidy up is best done as a precursor commit.

[romanchyla]

gpio.c exemplifies one other issue (eg: serout() function) - the code for checking/reading from Lua state is mixed with the execution logic. I.e. 80% of all lines deal with getting input arguments, 20% is doing the actual work. That may be a matter of taste or necessity (a smaller footprint?)

is there any 'official' guidance? (ie: why is better to have static int lgpio_serout( lua_State* L ) and not:

• static int lgpio_gateway( lua_State* L ) which calls
• static int serout(int pin, int pin_out, int pin_pull, int delay_table[])
  )

the issue is not only about readability, functions that declare their input arguments and return values are easier to test (and the code for reading/checking values could be extracted into a macro and reused by many modules - which would be actually saving space)

[TerryE]

I agree that it is well worth while enforcing a clear split by doing all of the API argument validation first, the execute the functional bit.

There is the issue of calling overhead both in terms of extra generated lx106 code and execution time to handle the procedural call overheads, but these can effectively removed by declaring the execution function first with an static inline attribute in which case the gcc compiler effectively treats this as a macro and you lose this overhead. It the routine then subsequently needs to be exported as an external function, it's then a simple issue to remove the inline declaration.

[marcelstoer]

Terry, I agree with you but is it worth keeping this issue open? When would be a good time to close it? Clearly we don't want to keep it around until all loose coding issues in all modules are fixed?

I suggest we close this and keep an eye on the issue with every PR that contains material changes.

[marcelstoer]

Bump. Don't wanna close your issues but I still think it should be closed.

[djphoenix]

I think the best case is to use linter like cpplint, this is good enough to get readable code (consistent tab/space struct, maximum line width and more). Also configuration for e.g. Eclipse Code Formatter will be good point. Anyway I prefer automated tools more than "guidelines".

[marcelstoer]

Yuri, that's a valid input - for #1168. Look at Terry's initial example here. That's not something a linter could "fix".

[karrots]

While eslint doesn't lint C/C++ code it is an example of a linter which will fix and/or identify exactly the issue. mentioned in JavaScript. Assuming I understand your inference correctly.

http://eslint.org/docs/rules/curly

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[TerryE]

Don't wanna close your issues but I still think it should be closed.

Nah, this is still very much the case. I am going through the modules ATM as part of the Lua51 + Lua53 integration and keep coming across this crap. It just adds noise to the modules that we don't need. For example every module which returns a UData resource like a socket registers a registry entry, say tls.socket, and uses a luaL_checkudata() on method entry to check that the US is of the correct type -- that is it has a metatable with is as required. A type error is thrown if there is a mismatch so the execution never gets to the following line if the userdata is invalid. So why do stuff like:

  luaL_argcheck(L, ud, 1, "TLS socket expected");
  if(ud==NULL){
      NODE_DBG("userdata is nil.\n");
      return 0;
  }

This is all totally redundant noise -- doubly redundant in this case -- and should be stripped out.

Likewise in the case of

  const char *method = luaL_checklstring( L, 2, &sl );
  if (method == NULL)
    return luaL_error( L, "wrong arg type" );

luaL_checklstring() either returns a pointer to a string or throws an error. It can never return NULL so why have the if statement and luaL_error()?

[TerryE]

What @nwf or I will do is a debris sweep to remove this crap.

[TerryE]

Reserving luaXXX names for the Lua runtime

A bunch of modules (bmp085.c, dcc.c, tsl2561.c, bme680.c, ads1115.c, si7021.c, tcs34725.c, bme280.c) create their own variables and functions with the lua prefix. All modules should reserve luaXXX for the lua API.

Call	bmp085.c	tsl2561.c	bme680.c	ads1115.c	si7021.c	tcs34725.c	bme280.c	Total Result
lua_altitude()				1				1
lua_baro()								1
lua_calclux()		1						1
lua_dewpoint()				1				1
lua_firmware()					1			1
lua_getchannels()		1						1
lua_humi()							1	1
lua_read()			1	1	1		1	4
lua_read()			1				1	4
lua_setting()				1	1			2
lua_setup()			1		1		1	4
lua_startread()				1				1
lua_startreadout()				1				1
lua_temp()			1				1
lua_temperature()	1							1
luaSetGain()						2		2
luaSetIntegrationTime()						2		2

The wifi modules also use lua_CB plus C variables