search

LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.77k stars 1.14k forks source link

[RFC] Change return value from `const char *` to `std::optional<const char*>` #2812

Closed havardAasen closed 7 months ago

havardAasen commented 8 months ago

This is a request for comments, or perhaps ask for approval, to change the return value for public functions in the library liblinuxcncini, specifically file src/libnml/inifile/inifile.hh. This change will break the API of the library, I will of course update the code within the project, but I'm unsure if, or how many, users who also use this library.

The proposed change is to return std::optional<const char*> instead of const char *.

This has several benefits, it tells the developer that the function can return something else than a const char *. The std::optional<>can be used as a boolean, if it has a value or not, which can simplify error handling. It is also increases type safety in regards to NULL pointer dereferencing.

andypugh commented 8 months ago

iniFind is also called from C code, does that cause problems. I think that I have heard (from Rene?) that we have multiple INI file parsers. There is the code in src/emc/ini for example. I am not sure which one Python uses if you import linuxcnc then operate on linuxcnc.ini

havardAasen commented 8 months ago

iniFind is also called from C code, does that cause problems.

Looking at mb2hal, which is one of the few parts that's using the C implementation, I don't see any difference. Which means that the tests still succeeds, tests that reads from an INI file.

I think that I have heard (from Rene?) that we have multiple INI file parsers. There is the code in src/emc/ini for example.

Yes, I found emcIniFile.hh which inherits from IniFile. The class EmcIniFile is almost untouched, at least the API is still the same.

I am not sure which one Python uses if you import linuxcnc then operate on linuxcnc.ini

Right, wasn't aware that Python also used it, will look into it.

I wrote some tests for the library, to make sure nothing breaks. I used inivar, which is written in C++, so C and Python is still untested.

Does this means you are mostly OK with the change, as long as the existing code doesn't break?

andypugh commented 8 months ago

If you think it makes things better, who am I to argue?