Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On 12 December 2015 at 15:41, Sudip Mukherjee wrote: > On Sat, Dec 12, 2015 at 11:40:25AM +0300, Dan Carpenter wrote: >> On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: >> > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: >> > > This patch does the following: >> > > * changed the complicated if statements to simple case statements >> > > * in case of E_DEFAULT, no need to return error as ERESTART, >> > > because this is the user asked for. Hence function should return success. >> > > * ret variable is 0 always, hence removed it. >> > > * removed one ternary operator, as it was always returning the status >> > > value only, >> > > and hence removed the status variable too >> > >> > That becomes 4 different changes. Please break them into separate >> > patches. >> >> It's cleaning up one function so you could argue that it's just one >> thing. > > Then maybe that should have been mentioned in the commit message along > with mentioning point wise the different changes made. > That is already mentioned in subject line of patch, which will be the part of commit message. Regrads, Saurabh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Sat, Dec 12, 2015 at 11:40:25AM +0300, Dan Carpenter wrote: > On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: > > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > > > This patch does the following: > > > * changed the complicated if statements to simple case statements > > > * in case of E_DEFAULT, no need to return error as ERESTART, > > > because this is the user asked for. Hence function should return success. > > > * ret variable is 0 always, hence removed it. > > > * removed one ternary operator, as it was always returning the status > > > value only, > > > and hence removed the status variable too > > > > That becomes 4 different changes. Please break them into separate > > patches. > > It's cleaning up one function so you could argue that it's just one > thing. Then maybe that should have been mentioned in the commit message along with mentioning point wise the different changes made. regards sudip -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On 12 December 2015 at 14:10, Dan Carpenter wrote: > On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: >> On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: >> > This patch does the following: >> > * changed the complicated if statements to simple case statements >> > * in case of E_DEFAULT, no need to return error as ERESTART, >> > because this is the user asked for. Hence function should return success. >> > * ret variable is 0 always, hence removed it. >> > * removed one ternary operator, as it was always returning the status >> > value only, >> > and hence removed the status variable too >> >> That becomes 4 different changes. Please break them into separate >> patches. > > It's cleaning up one function so you could argue that it's just one > thing. Sometimes it's actually harder to review when a patch is broken > into ultra tiny junks. yes Dan, this is my point too. I was planning to cleanup many function in speakup. If I will be breakup in to such a small chunk, it will problem for both of us. The best I can do is to hide the details in description and just say clanup there. I am open to suggestions. Regards, Saurabh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > > This patch does the following: > > * changed the complicated if statements to simple case statements > > * in case of E_DEFAULT, no need to return error as ERESTART, > > because this is the user asked for. Hence function should return success. > > * ret variable is 0 always, hence removed it. > > * removed one ternary operator, as it was always returning the status value > > only, > > and hence removed the status variable too > > That becomes 4 different changes. Please break them into separate > patches. It's cleaning up one function so you could argue that it's just one thing. Sometimes it's actually harder to review when a patch is broken into ultra tiny junks. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On 12 December 2015 at 14:10, Dan Carpenterwrote: > On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: >> On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: >> > This patch does the following: >> > * changed the complicated if statements to simple case statements >> > * in case of E_DEFAULT, no need to return error as ERESTART, >> > because this is the user asked for. Hence function should return success. >> > * ret variable is 0 always, hence removed it. >> > * removed one ternary operator, as it was always returning the status >> > value only, >> > and hence removed the status variable too >> >> That becomes 4 different changes. Please break them into separate >> patches. > > It's cleaning up one function so you could argue that it's just one > thing. Sometimes it's actually harder to review when a patch is broken > into ultra tiny junks. yes Dan, this is my point too. I was planning to cleanup many function in speakup. If I will be breakup in to such a small chunk, it will problem for both of us. The best I can do is to hide the details in description and just say clanup there. I am open to suggestions. Regards, Saurabh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > > This patch does the following: > > * changed the complicated if statements to simple case statements > > * in case of E_DEFAULT, no need to return error as ERESTART, > > because this is the user asked for. Hence function should return success. > > * ret variable is 0 always, hence removed it. > > * removed one ternary operator, as it was always returning the status value > > only, > > and hence removed the status variable too > > That becomes 4 different changes. Please break them into separate > patches. It's cleaning up one function so you could argue that it's just one thing. Sometimes it's actually harder to review when a patch is broken into ultra tiny junks. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Sat, Dec 12, 2015 at 11:40:25AM +0300, Dan Carpenter wrote: > On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: > > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > > > This patch does the following: > > > * changed the complicated if statements to simple case statements > > > * in case of E_DEFAULT, no need to return error as ERESTART, > > > because this is the user asked for. Hence function should return success. > > > * ret variable is 0 always, hence removed it. > > > * removed one ternary operator, as it was always returning the status > > > value only, > > > and hence removed the status variable too > > > > That becomes 4 different changes. Please break them into separate > > patches. > > It's cleaning up one function so you could argue that it's just one > thing. Then maybe that should have been mentioned in the commit message along with mentioning point wise the different changes made. regards sudip -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On 12 December 2015 at 15:41, Sudip Mukherjeewrote: > On Sat, Dec 12, 2015 at 11:40:25AM +0300, Dan Carpenter wrote: >> On Wed, Dec 09, 2015 at 10:47:18AM +0530, Sudip Mukherjee wrote: >> > On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: >> > > This patch does the following: >> > > * changed the complicated if statements to simple case statements >> > > * in case of E_DEFAULT, no need to return error as ERESTART, >> > > because this is the user asked for. Hence function should return success. >> > > * ret variable is 0 always, hence removed it. >> > > * removed one ternary operator, as it was always returning the status >> > > value only, >> > > and hence removed the status variable too >> > >> > That becomes 4 different changes. Please break them into separate >> > patches. >> >> It's cleaning up one function so you could argue that it's just one >> thing. > > Then maybe that should have been mentioned in the commit message along > with mentioning point wise the different changes made. > That is already mentioned in subject line of patch, which will be the part of commit message. Regrads, Saurabh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > This patch does the following: > * changed the complicated if statements to simple case statements > * in case of E_DEFAULT, no need to return error as ERESTART, > because this is the user asked for. Hence function should return success. > * ret variable is 0 always, hence removed it. > * removed one ternary operator, as it was always returning the status value > only, > and hence removed the status variable too That becomes 4 different changes. Please break them into separate patches. regards sudip -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
On Mon, Dec 07, 2015 at 06:35:11PM +0530, Saurabh Sengar wrote: > This patch does the following: > * changed the complicated if statements to simple case statements > * in case of E_DEFAULT, no need to return error as ERESTART, > because this is the user asked for. Hence function should return success. > * ret variable is 0 always, hence removed it. > * removed one ternary operator, as it was always returning the status value > only, > and hence removed the status variable too That becomes 4 different changes. Please break them into separate patches. regards sudip -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
This patch does the following: * changed the complicated if statements to simple case statements * in case of E_DEFAULT, no need to return error as ERESTART, because this is the user asked for. Hence function should return success. * ret variable is 0 always, hence removed it. * removed one ternary operator, as it was always returning the status value only, and hence removed the status variable too Signed-off-by: Saurabh Sengar --- drivers/staging/speakup/varhandlers.c | 50 +-- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c index ab4fe8d..e1393d2 100644 --- a/drivers/staging/speakup/varhandlers.c +++ b/drivers/staging/speakup/varhandlers.c @@ -176,7 +176,6 @@ struct punc_var_t *spk_get_punc_var(enum var_id_t var_id) int spk_set_num_var(int input, struct st_var_header *var, int how) { int val; - short ret = 0; int *p_val = var->p_val; int l; char buf[32]; @@ -186,50 +185,51 @@ int spk_set_num_var(int input, struct st_var_header *var, int how) if (!var_data) return -ENODATA; - if (how == E_NEW_DEFAULT) { + val = var_data->u.n.value; + switch (how) { + case E_NEW_DEFAULT: if (input < var_data->u.n.low || input > var_data->u.n.high) return -ERANGE; var_data->u.n.default_val = input; return 0; - } - if (how == E_DEFAULT) { + case E_DEFAULT: val = var_data->u.n.default_val; - ret = -ERESTART; - } else { - if (how == E_SET) - val = input; - else - val = var_data->u.n.value; - if (how == E_INC) - val += input; - else if (how == E_DEC) - val -= input; - if (val < var_data->u.n.low || val > var_data->u.n.high) - return -ERANGE; + break; + case E_SET: + val = input; + break; + case E_INC: + val += input; + break; + case E_DEC: + val -= input; + break; } + + if (val < var_data->u.n.low || val > var_data->u.n.high) + return -ERANGE; + var_data->u.n.value = val; if (var->var_type == VAR_TIME && p_val != NULL) { *p_val = msecs_to_jiffies(val); - return ret; + return 0; } if (p_val != NULL) *p_val = val; if (var->var_id == PUNC_LEVEL) { spk_punc_mask = spk_punc_masks[val]; - return ret; + return 0; } if (var_data->u.n.multiplier != 0) val *= var_data->u.n.multiplier; val += var_data->u.n.offset; if (var->var_id < FIRST_SYNTH_VAR || !synth) - return ret; - if (synth->synth_adjust) { - int status = synth->synth_adjust(var); + return 0; + if (synth->synth_adjust) + return synth->synth_adjust(var); - return (status != 0) ? status : ret; - } if (!var_data->u.n.synth_fmt) - return ret; + return 0; if (var->var_id == PITCH) cp = spk_pitch_buff; else @@ -240,7 +240,7 @@ int spk_set_num_var(int input, struct st_var_header *var, int how) l = sprintf(cp, var_data->u.n.synth_fmt, var_data->u.n.out_str[val]); synth_printf("%s", cp); - return ret; + return 0; } int spk_set_string_var(const char *page, struct st_var_header *var, int len) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: speakup: varhandlers: cleanup of function spk_get_punc_var
This patch does the following: * changed the complicated if statements to simple case statements * in case of E_DEFAULT, no need to return error as ERESTART, because this is the user asked for. Hence function should return success. * ret variable is 0 always, hence removed it. * removed one ternary operator, as it was always returning the status value only, and hence removed the status variable too Signed-off-by: Saurabh Sengar--- drivers/staging/speakup/varhandlers.c | 50 +-- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c index ab4fe8d..e1393d2 100644 --- a/drivers/staging/speakup/varhandlers.c +++ b/drivers/staging/speakup/varhandlers.c @@ -176,7 +176,6 @@ struct punc_var_t *spk_get_punc_var(enum var_id_t var_id) int spk_set_num_var(int input, struct st_var_header *var, int how) { int val; - short ret = 0; int *p_val = var->p_val; int l; char buf[32]; @@ -186,50 +185,51 @@ int spk_set_num_var(int input, struct st_var_header *var, int how) if (!var_data) return -ENODATA; - if (how == E_NEW_DEFAULT) { + val = var_data->u.n.value; + switch (how) { + case E_NEW_DEFAULT: if (input < var_data->u.n.low || input > var_data->u.n.high) return -ERANGE; var_data->u.n.default_val = input; return 0; - } - if (how == E_DEFAULT) { + case E_DEFAULT: val = var_data->u.n.default_val; - ret = -ERESTART; - } else { - if (how == E_SET) - val = input; - else - val = var_data->u.n.value; - if (how == E_INC) - val += input; - else if (how == E_DEC) - val -= input; - if (val < var_data->u.n.low || val > var_data->u.n.high) - return -ERANGE; + break; + case E_SET: + val = input; + break; + case E_INC: + val += input; + break; + case E_DEC: + val -= input; + break; } + + if (val < var_data->u.n.low || val > var_data->u.n.high) + return -ERANGE; + var_data->u.n.value = val; if (var->var_type == VAR_TIME && p_val != NULL) { *p_val = msecs_to_jiffies(val); - return ret; + return 0; } if (p_val != NULL) *p_val = val; if (var->var_id == PUNC_LEVEL) { spk_punc_mask = spk_punc_masks[val]; - return ret; + return 0; } if (var_data->u.n.multiplier != 0) val *= var_data->u.n.multiplier; val += var_data->u.n.offset; if (var->var_id < FIRST_SYNTH_VAR || !synth) - return ret; - if (synth->synth_adjust) { - int status = synth->synth_adjust(var); + return 0; + if (synth->synth_adjust) + return synth->synth_adjust(var); - return (status != 0) ? status : ret; - } if (!var_data->u.n.synth_fmt) - return ret; + return 0; if (var->var_id == PITCH) cp = spk_pitch_buff; else @@ -240,7 +240,7 @@ int spk_set_num_var(int input, struct st_var_header *var, int how) l = sprintf(cp, var_data->u.n.synth_fmt, var_data->u.n.out_str[val]); synth_printf("%s", cp); - return ret; + return 0; } int spk_set_string_var(const char *page, struct st_var_header *var, int len) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/