| Message ID | 20171004155700.18048-2-christoph@boehmwalder.at (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Luca Coelho |
| Headers | show |
On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote: > Change a usage of int in a boolean context to use the bool type > instead, as it > makes the intent of the function clearer and helps clarify its > semantics. > > Also eliminate the if/else and just return the boolean result > directly, > making the code more readable. > > Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at> > --- > drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > index b7cd813ba70f..0eb815ae97e8 100644 > --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db > *phy_db, > } > IWL_EXPORT_SYMBOL(iwl_phy_db_set_section); > > -static int is_valid_channel(u16 ch_id) > +static bool is_valid_channel(u16 ch_id) > { > - if (ch_id <= 14 || > - (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > - (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > - (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)) > - return 1; > - return 0; > + return (ch_id <= 14 || > + (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > + (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > + (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)); > } > > static u8 ch_id_to_ch_index(u16 ch_id) This actually makes some sense, and I would probably apply it if it were part of a patchset that actually does something useful. -- Luca.
On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote: > Change a usage of int in a boolean context to use the bool type instead, as it > makes the intent of the function clearer and helps clarify its semantics. > > Also eliminate the if/else and just return the boolean result directly, > making the code more readable. > > Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at> > --- > drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > index b7cd813ba70f..0eb815ae97e8 100644 > --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db *phy_db, > } > IWL_EXPORT_SYMBOL(iwl_phy_db_set_section); > > -static int is_valid_channel(u16 ch_id) > +static bool is_valid_channel(u16 ch_id) > { > - if (ch_id <= 14 || > - (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > - (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > - (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)) > - return 1; > - return 0; > + return (ch_id <= 14 || > + (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > + (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > + (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)); > } This might be more intelligble as separate tests static bool is_valid_channel(u16 ch_id) { if (ch_id <= 14) return true; if ((ch_id % 4 == 0) && ((ch_id >= 36 && ch_id <= 64) || (ch_id >= 100 && ch_id <= 140))) return true; if ((ch_id % 4 == 1) && (chid >= 145 && ch_id <= 165)) return true; return false; } The compiler should produce the same object code.
On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote: > On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote: > > Change a usage of int in a boolean context to use the bool type > > instead, as it > > makes the intent of the function clearer and helps clarify its > > semantics. > > > > Also eliminate the if/else and just return the boolean result > > directly, > > making the code more readable. > > > > Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at> > > --- > > drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > > b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > > index b7cd813ba70f..0eb815ae97e8 100644 > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c > > @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db > > *phy_db, > > } > > IWL_EXPORT_SYMBOL(iwl_phy_db_set_section); > > > > -static int is_valid_channel(u16 ch_id) > > +static bool is_valid_channel(u16 ch_id) > > { > > - if (ch_id <= 14 || > > - (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > > - (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > > - (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)) > > - return 1; > > - return 0; > > + return (ch_id <= 14 || > > + (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || > > + (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || > > + (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)); > > } > > This might be more intelligble as separate tests > > static bool is_valid_channel(u16 ch_id) > { > if (ch_id <= 14) > return true; > > if ((ch_id % 4 == 0) && > ((ch_id >= 36 && ch_id <= 64) || > (ch_id >= 100 && ch_id <= 140))) > return true; > > if ((ch_id % 4 == 1) && > (chid >= 145 && ch_id <= 165)) > return true; > > return false; > } > > The compiler should produce the same object code. Yeah, it may be a bit easier to read, but I don't want to start getting "fixes" to working and reasonable code. There's nothing wrong with the existing function (except maybe for the int vs. boolean) so let's not change it. A good time to change this would be the next time someone adds yet another range of valid channels here. ;) -- Luca.
On Wed, 2017-10-04 at 19:39 +0300, Luciano Coelho wrote: > On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote: [] > > This might be more intelligble as separate tests > > > > static bool is_valid_channel(u16 ch_id) > > { > > if (ch_id <= 14) > > return true; > > > > if ((ch_id % 4 == 0) && > > ((ch_id >= 36 && ch_id <= 64) || > > (ch_id >= 100 && ch_id <= 140))) > > return true; > > > > if ((ch_id % 4 == 1) && > > (chid >= 145 && ch_id <= 165)) > > return true; > > > > return false; > > } > > > > The compiler should produce the same object code. > > Yeah, it may be a bit easier to read, but I don't want to start getting > "fixes" to working and reasonable code. There's nothing wrong with the > existing function (except maybe for the int vs. boolean) so let's not > change it. > > A good time to change this would be the next time someone adds yet > another range of valid channels here. ;) <shrug> Your choice. I like code I can read and understand at a glance. At case somebody needs to add channels, likely nobody would do the change suggested but would just add another test to the already odd looking block. And constants should be on the right side of the tests.
On Wed, 2017-10-04 at 10:55 -0700, Joe Perches wrote: > On Wed, 2017-10-04 at 19:39 +0300, Luciano Coelho wrote: > > On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote: > > [] > > > This might be more intelligble as separate tests > > > > > > static bool is_valid_channel(u16 ch_id) > > > { > > > if (ch_id <= 14) > > > return true; > > > > > > if ((ch_id % 4 == 0) && > > > ((ch_id >= 36 && ch_id <= 64) || > > > (ch_id >= 100 && ch_id <= 140))) > > > return true; > > > > > > if ((ch_id % 4 == 1) && > > > (chid >= 145 && ch_id <= 165)) > > > return true; > > > > > > return false; > > > } > > > > > > The compiler should produce the same object code. > > > > Yeah, it may be a bit easier to read, but I don't want to start > > getting > > "fixes" to working and reasonable code. There's nothing wrong with > > the > > existing function (except maybe for the int vs. boolean) so let's > > not > > change it. > > > > A good time to change this would be the next time someone adds yet > > another range of valid channels here. ;) > > <shrug> Your choice. > > I like code I can read and understand at a glance. I do too, but I don't think the original is that hard to read, really. Each "if" you add is already corresponding to one separate line in the original code... > At case somebody needs to add channels, likely nobody > would do the change suggested but would just add > another test to the already odd looking block. Yeah, that would most likely be the case, but if I saw that and thought there was a better way to write it, believe me, I would definitely nitpick the patch and ask the author to reorg the code so it would look nicer. > And constants should be on the right side of the tests. Sure, in a new patch, we would definitely pay attention to that. But now, is it worth having one more patch go through the entire machinery to change a relatively clear, extremely simple function just because you could write it in a different way? My answer is a resounding no, sorry. -- Luca.
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c index b7cd813ba70f..0eb815ae97e8 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db *phy_db, } IWL_EXPORT_SYMBOL(iwl_phy_db_set_section); -static int is_valid_channel(u16 ch_id) +static bool is_valid_channel(u16 ch_id) { - if (ch_id <= 14 || - (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || - (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || - (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)) - return 1; - return 0; + return (ch_id <= 14 || + (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) || + (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) || + (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1)); } static u8 ch_id_to_ch_index(u16 ch_id)
Change a usage of int in a boolean context to use the bool type instead, as it makes the intent of the function clearer and helps clarify its semantics. Also eliminate the if/else and just return the boolean result directly, making the code more readable. Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at> --- drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)