Messages in this thread | | | Subject | Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h | From | Marcus Wolf <> | Date | Mon, 4 Dec 2017 21:22:06 +0200 |
| |
Am 04.12.2017 um 21:15 schrieb Dan Carpenter: > On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote: >> >> >> Am 04.12.2017 um 12:37 schrieb Dan Carpenter: >>> On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote: >>>> Perhaps choose different function names if you want? You could do it >>>> as several patches: >>>> >>>> patch 1: change types to bool >>>> patch 2: sed -e '/ == optionOn//' >>>> patch 3: split the functions into two functions >>>> patch 4: delete optionOnOff enum >>>> >>>> patches 1 and 2 could be merged together (your choice). >>>> >>> >>> Markus says that optionOn is used by user space so my you won't be able >>> to remove these entirely. But as much as possible we should internally. >>> >>> regards, >>> dan carpenter >>> >> >> Hi Dan, hi Simon, >> >> I think, it's a pretty nice idea to remove th optionOnOff and replace it by >> bool. >> >> <history> >> In former times, the variables in the config struct had very different names >> - not containing "enable". Therefore optionOnOff was used to make absolutely >> clear (in user space), wheter something was switched on, or off. >> Now the variable have nice names, so bool is fine, even better now :-) >> </history> >> >> I would suggest not to split the amp-functions but to rename them, to also >> contain an enable: >> rf69_set_amp_X_enable() > > That's a bad name, because it doesn't just enable it also disables. > Please split them. > > regards, > dan carpenter > >
Same applies to all other stuff, that's using optionOnOff: rf69_set_sync_enable(optionOn/Off) enables and disbales sync, rf69_set_crc_enable(optionOn/Off) enables and disables crc, ...
In my opinion, if we want perfect clarity, we should stay with optionOnOff. If we are ok, if rf69_set_sync_enable(false) disables sync, in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false) disables the amp.
Cheers, Marcus
| |