* Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double underscores from states names
[not found] <20230419231751.11581-1-ahmed.zaki@intel.com>
@ 2023-04-20 14:33 ` Alexander Lobakin
2023-04-20 14:59 ` Jakub Kicinski
0 siblings, 1 reply; 2+ messages in thread
From: Alexander Lobakin @ 2023-04-20 14:33 UTC (permalink / raw)
To: Ahmed Zaki; +Cc: intel-wired-lan, Jakub Kicinski, netdev@vger.kernel.org
From: Ahmed Zaki <ahmed.zaki@intel.com>
Date: Wed, 19 Apr 2023 17:17:51 -0600
+ netdev
Please add netdev ML next time you send networking stuff, not only Jakub :D
> No need to prepend some states names with "__", especially that others do
> not have these underscores.
What's the value of this change, apart from 400 locs diffstat? I don't
say having prefixes for general-purpose definitions is correct, but...
Dunno. Would be nice to have as a part of some series with assorted
improvements, but not standalone. Or it's just me =\
>
> Link: https://lore.kernel.org/netdev/20230406130415.1aa1d1cd@kernel.org
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
[...]
> @@ -468,36 +468,36 @@ extern char iavf_driver_name[];
> static inline const char *iavf_state_str(enum iavf_state_t state)
> {
> switch (state) {
> - case __IAVF_STARTUP:
> - return "__IAVF_STARTUP";
> - case __IAVF_REMOVE:
> - return "__IAVF_REMOVE";
> - case __IAVF_INIT_VERSION_CHECK:
> - return "__IAVF_INIT_VERSION_CHECK";
> - case __IAVF_INIT_GET_RESOURCES:
> - return "__IAVF_INIT_GET_RESOURCES";
> - case __IAVF_INIT_EXTENDED_CAPS:
> - return "__IAVF_INIT_EXTENDED_CAPS";
> - case __IAVF_INIT_CONFIG_ADAPTER:
> - return "__IAVF_INIT_CONFIG_ADAPTER";
> - case __IAVF_INIT_SW:
> - return "__IAVF_INIT_SW";
> - case __IAVF_INIT_FAILED:
> - return "__IAVF_INIT_FAILED";
> - case __IAVF_RESETTING:
> - return "__IAVF_RESETTING";
> - case __IAVF_COMM_FAILED:
> - return "__IAVF_COMM_FAILED";
> - case __IAVF_DOWN:
> - return "__IAVF_DOWN";
> - case __IAVF_DOWN_PENDING:
> - return "__IAVF_DOWN_PENDING";
> - case __IAVF_TESTING:
> - return "__IAVF_TESTING";
> - case __IAVF_RUNNING:
> - return "__IAVF_RUNNING";
> + case IAVF_STARTUP:
> + return "IAVF_STARTUP";
> + case IAVF_REMOVE:
> + return "IAVF_REMOVE";
> + case IAVF_INIT_VERSION_CHECK:
> + return "IAVF_INIT_VERSION_CHECK";
> + case IAVF_INIT_GET_RESOURCES:
> + return "IAVF_INIT_GET_RESOURCES";
> + case IAVF_INIT_EXTENDED_CAPS:
> + return "IAVF_INIT_EXTENDED_CAPS";
> + case IAVF_INIT_CONFIG_ADAPTER:
> + return "IAVF_INIT_CONFIG_ADAPTER";
> + case IAVF_INIT_SW:
> + return "IAVF_INIT_SW";
> + case IAVF_INIT_FAILED:
> + return "IAVF_INIT_FAILED";
> + case IAVF_RESETTING:
> + return "IAVF_RESETTING";
> + case IAVF_COMM_FAILED:
> + return "IAVF_COMM_FAILED";
> + case IAVF_DOWN:
> + return "IAVF_DOWN";
> + case IAVF_DOWN_PENDING:
> + return "IAVF_DOWN_PENDING";
> + case IAVF_TESTING:
> + return "IAVF_TESTING";
> + case IAVF_RUNNING:
> + return "IAVF_RUNNING";
> default:
> - return "__IAVF_UNKNOWN_STATE";
> + return "IAVF_UNKNOWN_STATE";
> }
> }
If you want to do something really useful, pls replace that non-sense
with O(1) array lookup:
iavf_something.c:
const char * const iavf_states[] = {
#define __IAVF_STATE_STR(s) [IAVF_##s] = "IAVF_" #s
__IAVF_STATE_STR(STARTUP),
__IAVF_STATE_STR(REMOVE),
...
__IAVF_STATE_STR(RUNNING),
#undef __IAVF_STATE_STR
"IAVF_UNKNOWN_STATE",
}
iavf.h:
extern const char * const iavf_states[];
static inline const char *iavf_state_str(enum iavf_state_t state)
{
if (unlikely(state > IAVF_STATE_RUNNING))
state = IAVF_STATE_RUNNING + 1;
// ^ or better just add `IAVF_UNKNOWN_STATE` to the enum
// to not open-code `RUNNING + 1`
return iavf_states[state];
}
You can even just copy'n'paste that and make a patch, I think it would
work :D
And attach bloat-o-meter output to the commit message, I'd like to watch
the result :3
[...]
Thanks,
Olek
^ permalink raw reply [flat|nested] 2+ messages in thread