[PATCH 0/2] staging: speakup: i18n.c: Refactor conditionals and change return type

[PATCH 0/2] staging: speakup: i18n.c: Refactor conditionals and change return type

[Narcisa Ana Maria Vasile]

Cc: outreachy-kernel@googlegroups.com

Change return type from int to bool and refactor code

Narcisa Ana Maria Vasile (2):
  staging: speakup: i18n: Change return value
  staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

 drivers/staging/speakup/i18n.c | 60 ++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

--
1.9.1

[PATCH 1/2] staging: speakup: i18n: Change return value

[Narcisa Ana Maria Vasile]

Change return value from int to bool in compare_specifiers
and fmt_validate functions. The possible return values (0 or 1)
for these functions represent whether a condition holds or not, so
conceptually, they are booleans.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index 1a3e348..00ffa4e 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -479,16 +479,16 @@ static char *find_specifier_end(char *input)
  * Return 1 if they are the same, 0 otherwise.  Advance *input1 and *input2
  * so that they point to the character following the end of the specifier.
  */
-static int compare_specifiers(char **input1, char **input2)
+static bool compare_specifiers(char **input1, char **input2)
 {
-	int same = 0;
+	bool same = false;
 	char *end1 = find_specifier_end(*input1);
 	char *end2 = find_specifier_end(*input2);
 	size_t length1 = end1 - *input1;
 	size_t length2 = end2 - *input2;

 	if ((length1 == length2) && !memcmp(*input1, *input2, length1))
-		same = 1;
+		same = true;

 	*input1 = end1;
 	*input2 = end2;
@@ -501,10 +501,10 @@ static int compare_specifiers(char **input1, char **input2)
  * and that the order of specifiers is the same in both strings.
  * Return 1 if the condition holds, 0 if it doesn't.
  */
-static int fmt_validate(char *template, char *user)
+static bool fmt_validate(char *template, char *user)
 {
-	int valid = 1;
-	int still_comparing = 1;
+	bool valid = true;
+	bool still_comparing = true;
 	char *template_ptr = template;
 	char *user_ptr = user;

@@ -516,10 +516,10 @@ static int fmt_validate(char *template, char *user)
 			valid = compare_specifiers(&template_ptr, &user_ptr);
 		} else {
 			/* No more format specifiers in one or both strings. */
-			still_comparing = 0;
+			still_comparing = false;
 			/* See if one has more specifiers than the other. */
 			if (template_ptr || user_ptr)
-				valid = 0;
+				valid = false;
 		}
 	}
 	return valid;
--
1.9.1

[PATCH 2/2] staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

[Narcisa Ana Maria Vasile]

Reduce the indentation level in the conditionals in spk_msg_set
and remove unnecessary return variable.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index 00ffa4e..a5b343c 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -540,34 +540,30 @@ static bool fmt_validate(char *template, char *user)
  */
 ssize_t spk_msg_set(enum msg_index_t index, char *text, size_t length)
 {
-	int rc = 0;
 	char *newstr = NULL;
 	unsigned long flags;

-	if ((index >= MSG_FIRST_INDEX) && (index < MSG_LAST_INDEX)) {
-		newstr = kmalloc(length + 1, GFP_KERNEL);
-		if (newstr) {
-			memcpy(newstr, text, length);
-			newstr[length] = '\0';
-			if (index >= MSG_FORMATTED_START &&
-			    index <= MSG_FORMATTED_END &&
-			    !fmt_validate(speakup_default_msgs[index],
-			                  newstr)) {
-				kfree(newstr);
-				return -EINVAL;
-			}
-			spin_lock_irqsave(&speakup_info.spinlock, flags);
-			if (speakup_msgs[index] != speakup_default_msgs[index])
-				kfree(speakup_msgs[index]);
-			speakup_msgs[index] = newstr;
-			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
-		} else {
-			rc = -ENOMEM;
-		}
-	} else {
-		rc = -EINVAL;
+	if (!(index >= MSG_FIRST_INDEX) || !(index < MSG_LAST_INDEX))
+		return -EINVAL;
+	newstr = kmalloc(length + 1, GFP_KERNEL);
+	if (!newstr)
+		return -ENOMEM;
+
+	memcpy(newstr, text, length);
+	newstr[length] = '\0';
+	if (index >= MSG_FORMATTED_START &&
+	    index <= MSG_FORMATTED_END &&
+	    !fmt_validate(speakup_default_msgs[index],
+	                  newstr)) {
+		kfree(newstr);
+		return -EINVAL;
 	}
-	return rc;
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (speakup_msgs[index] != speakup_default_msgs[index])
+		kfree(speakup_msgs[index]);
+	speakup_msgs[index] = newstr;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+	return 0;
 }

 /*
--
1.9.1

Re: [Outreachy kernel] [PATCH 2/2] staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

[Julia Lawall]

On Thu, 9 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Reduce the indentation level in the conditionals in spk_msg_set
> and remove unnecessary return variable.
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> ---
>  drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
> index 00ffa4e..a5b343c 100644
> --- a/drivers/staging/speakup/i18n.c
> +++ b/drivers/staging/speakup/i18n.c
> @@ -540,34 +540,30 @@ static bool fmt_validate(char *template, char *user)
>   */
>  ssize_t spk_msg_set(enum msg_index_t index, char *text, size_t length)
>  {
> -	int rc = 0;
>  	char *newstr = NULL;
>  	unsigned long flags;
>
> -	if ((index >= MSG_FIRST_INDEX) && (index < MSG_LAST_INDEX)) {
> -		newstr = kmalloc(length + 1, GFP_KERNEL);
> -		if (newstr) {
> -			memcpy(newstr, text, length);
> -			newstr[length] = '\0';
> -			if (index >= MSG_FORMATTED_START &&
> -			    index <= MSG_FORMATTED_END &&
> -			    !fmt_validate(speakup_default_msgs[index],
> -			                  newstr)) {
> -				kfree(newstr);
> -				return -EINVAL;
> -			}
> -			spin_lock_irqsave(&speakup_info.spinlock, flags);
> -			if (speakup_msgs[index] != speakup_default_msgs[index])
> -				kfree(speakup_msgs[index]);
> -			speakup_msgs[index] = newstr;
> -			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> -		} else {
> -			rc = -ENOMEM;
> -		}
> -	} else {
> -		rc = -EINVAL;
> +	if (!(index >= MSG_FIRST_INDEX) || !(index < MSG_LAST_INDEX))
> +		return -EINVAL;

You can push the ! inwards here.

> +	newstr = kmalloc(length + 1, GFP_KERNEL);
> +	if (!newstr)
> +		return -ENOMEM;
> +
> +	memcpy(newstr, text, length);
> +	newstr[length] = '\0';
> +	if (index >= MSG_FORMATTED_START &&
> +	    index <= MSG_FORMATTED_END &&
> +	    !fmt_validate(speakup_default_msgs[index],
> +	                  newstr)) {

This would all fit on one line now.

julia

> +		kfree(newstr);
> +		return -EINVAL;
>  	}
> -	return rc;
> +	spin_lock_irqsave(&speakup_info.spinlock, flags);
> +	if (speakup_msgs[index] != speakup_default_msgs[index])
> +		kfree(speakup_msgs[index]);
> +	speakup_msgs[index] = newstr;
> +	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +	return 0;
>  }
>
>  /*
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/2f91d0d2b5d05a6be6191b3090507106257ae05a.1489047172.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 1/2] staging: speakup: i18n: Change return value

[Julia Lawall]

On Thu, 9 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Change return value from int to bool in compare_specifiers
> and fmt_validate functions. The possible return values (0 or 1)
> for these functions represent whether a condition holds or not, so
> conceptually, they are booleans.
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> ---
>  drivers/staging/speakup/i18n.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
> index 1a3e348..00ffa4e 100644
> --- a/drivers/staging/speakup/i18n.c
> +++ b/drivers/staging/speakup/i18n.c
> @@ -479,16 +479,16 @@ static char *find_specifier_end(char *input)
>   * Return 1 if they are the same, 0 otherwise.  Advance *input1 and *input2
>   * so that they point to the character following the end of the specifier.
>   */

You need to update the documentation, for this function and the next one.

julia

> -static int compare_specifiers(char **input1, char **input2)
> +static bool compare_specifiers(char **input1, char **input2)
>  {
> -	int same = 0;
> +	bool same = false;
>  	char *end1 = find_specifier_end(*input1);
>  	char *end2 = find_specifier_end(*input2);
>  	size_t length1 = end1 - *input1;
>  	size_t length2 = end2 - *input2;
>
>  	if ((length1 == length2) && !memcmp(*input1, *input2, length1))
> -		same = 1;
> +		same = true;
>
>  	*input1 = end1;
>  	*input2 = end2;
> @@ -501,10 +501,10 @@ static int compare_specifiers(char **input1, char **input2)
>   * and that the order of specifiers is the same in both strings.
>   * Return 1 if the condition holds, 0 if it doesn't.
>   */
> -static int fmt_validate(char *template, char *user)
> +static bool fmt_validate(char *template, char *user)
>  {
> -	int valid = 1;
> -	int still_comparing = 1;
> +	bool valid = true;
> +	bool still_comparing = true;
>  	char *template_ptr = template;
>  	char *user_ptr = user;
>
> @@ -516,10 +516,10 @@ static int fmt_validate(char *template, char *user)
>  			valid = compare_specifiers(&template_ptr, &user_ptr);
>  		} else {
>  			/* No more format specifiers in one or both strings. */
> -			still_comparing = 0;
> +			still_comparing = false;
>  			/* See if one has more specifiers than the other. */
>  			if (template_ptr || user_ptr)
> -				valid = 0;
> +				valid = false;
>  		}
>  	}
>  	return valid;
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/9214e6265b0f7d95ac905107fa1b69b3645f78b1.1489047172.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

[PATCH v2 0/2] staging: speakup: i18n.c: Refactor conditionals and change return type

[Narcisa Ana Maria Vasile]

Change return type from int to bool and refactor code.

---
Changes in v2:
    - Update documentation for compare_specifiers and fmt_validate
    - Make commit messages more clear
    - Rewrite condition without using '!'
    - Fit function call on single line

Narcisa Ana Maria Vasile (2):
  staging: speakup: i18n: Change return type from int to bool
  staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

 drivers/staging/speakup/i18n.c | 67 ++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] staging: speakup: i18n: Change return type from int to bool

[Narcisa Ana Maria Vasile]

The possible return values (0 or 1) for compare_specifiers
and fmt_validate represent whether a condition holds or not, so
conceptually, they are booleans.

Update documentation for these two functions.

Change type of variable 'still_comparing' from int to bool too,
inside fmt_validate, because it is intended to hold truth values
of logic as well.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index ac1ebea..522d719 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -476,19 +476,20 @@ static char *find_specifier_end(char *input)
 /*
  * Function: compare_specifiers
  * Compare the format specifiers pointed to by *input1 and *input2.
- * Return 1 if they are the same, 0 otherwise.  Advance *input1 and *input2
- * so that they point to the character following the end of the specifier.
+ * Return true if they are the same, false otherwise.
+ * Advance *input1 and *input2 so that they point to the character following
+ * the end of the specifier.
  */
-static int compare_specifiers(char **input1, char **input2)
+static bool compare_specifiers(char **input1, char **input2)
 {
-	int same = 0;
+	bool same = false;
 	char *end1 = find_specifier_end(*input1);
 	char *end2 = find_specifier_end(*input2);
 	size_t length1 = end1 - *input1;
 	size_t length2 = end2 - *input2;
 
 	if ((length1 == length2) && !memcmp(*input1, *input2, length1))
-		same = 1;
+		same = true;
 
 	*input1 = end1;
 	*input2 = end2;
@@ -499,12 +500,12 @@ static int compare_specifiers(char **input1, char **input2)
  * Function: fmt_validate
  * Check that two format strings contain the same number of format specifiers,
  * and that the order of specifiers is the same in both strings.
- * Return 1 if the condition holds, 0 if it doesn't.
+ * Return true if the condition holds, false if it doesn't.
  */
-static int fmt_validate(char *template, char *user)
+static bool fmt_validate(char *template, char *user)
 {
-	int valid = 1;
-	int still_comparing = 1;
+	bool valid = 1;
+	bool still_comparing = 1;
 	char *template_ptr = template;
 	char *user_ptr = user;
 
@@ -516,10 +517,10 @@ static int fmt_validate(char *template, char *user)
 			valid = compare_specifiers(&template_ptr, &user_ptr);
 		} else {
 			/* No more format specifiers in one or both strings. */
-			still_comparing = 0;
+			still_comparing = false;
 			/* See if one has more specifiers than the other. */
 			if (template_ptr || user_ptr)
-				valid = 0;
+				valid = false;
 		}
 	}
 	return valid;
-- 
1.9.1

[PATCH v2 2/2] staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

[Narcisa Ana Maria Vasile]

Reduce the indentation level in spk_msg_set and remove
unnecessary return variable.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index 522d719..180dee3 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -541,34 +541,30 @@ static bool fmt_validate(char *template, char *user)
  */
 ssize_t spk_msg_set(enum msg_index_t index, char *text, size_t length)
 {
-	int rc = 0;
 	char *newstr = NULL;
 	unsigned long flags;
 
-	if ((index >= MSG_FIRST_INDEX) && (index < MSG_LAST_INDEX)) {
-		newstr = kmalloc(length + 1, GFP_KERNEL);
-		if (newstr) {
-			memcpy(newstr, text, length);
-			newstr[length] = '\0';
-			if (index >= MSG_FORMATTED_START &&
-			    index <= MSG_FORMATTED_END &&
-			    !fmt_validate(speakup_default_msgs[index],
-					  newstr)) {
-				kfree(newstr);
-				return -EINVAL;
-			}
-			spin_lock_irqsave(&speakup_info.spinlock, flags);
-			if (speakup_msgs[index] != speakup_default_msgs[index])
-				kfree(speakup_msgs[index]);
-			speakup_msgs[index] = newstr;
-			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
-		} else {
-			rc = -ENOMEM;
-		}
-	} else {
-		rc = -EINVAL;
+	if ((index < MSG_FIRST_INDEX) || (index >= MSG_LAST_INDEX))
+		return -EINVAL;
+
+	newstr = kmalloc(length + 1, GFP_KERNEL);
+	if (!newstr)
+		return -ENOMEM;
+
+	memcpy(newstr, text, length);
+	newstr[length] = '\0';
+	if (index >= MSG_FORMATTED_START &&
+	    index <= MSG_FORMATTED_END &&
+	    !fmt_validate(speakup_default_msgs[index], newstr)) {
+		kfree(newstr);
+		return -EINVAL;
 	}
-	return rc;
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (speakup_msgs[index] != speakup_default_msgs[index])
+		kfree(speakup_msgs[index]);
+	speakup_msgs[index] = newstr;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+	return 0;
 }
 
 /*
-- 
1.9.1

Re: [Outreachy kernel] [PATCH v2 1/2] staging: speakup: i18n: Change return type from int to bool

[Julia Lawall]

On Thu, 9 Mar 2017, Narcisa Ana Maria Vasile wrote:

> The possible return values (0 or 1) for compare_specifiers
> and fmt_validate represent whether a condition holds or not, so
> conceptually, they are booleans.
>
> Update documentation for these two functions.
>
> Change type of variable 'still_comparing' from int to bool too,
> inside fmt_validate, because it is intended to hold truth values
> of logic as well.
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
> ---
>  drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
> index ac1ebea..522d719 100644
> --- a/drivers/staging/speakup/i18n.c
> +++ b/drivers/staging/speakup/i18n.c
> @@ -476,19 +476,20 @@ static char *find_specifier_end(char *input)
>  /*
>   * Function: compare_specifiers
>   * Compare the format specifiers pointed to by *input1 and *input2.
> - * Return 1 if they are the same, 0 otherwise.  Advance *input1 and *input2
> - * so that they point to the character following the end of the specifier.
> + * Return true if they are the same, false otherwise.
> + * Advance *input1 and *input2 so that they point to the character following
> + * the end of the specifier.
>   */
> -static int compare_specifiers(char **input1, char **input2)
> +static bool compare_specifiers(char **input1, char **input2)
>  {
> -	int same = 0;
> +	bool same = false;
>  	char *end1 = find_specifier_end(*input1);
>  	char *end2 = find_specifier_end(*input2);
>  	size_t length1 = end1 - *input1;
>  	size_t length2 = end2 - *input2;
>
>  	if ((length1 == length2) && !memcmp(*input1, *input2, length1))
> -		same = 1;
> +		same = true;
>
>  	*input1 = end1;
>  	*input2 = end2;
> @@ -499,12 +500,12 @@ static int compare_specifiers(char **input1, char **input2)
>   * Function: fmt_validate
>   * Check that two format strings contain the same number of format specifiers,
>   * and that the order of specifiers is the same in both strings.
> - * Return 1 if the condition holds, 0 if it doesn't.
> + * Return true if the condition holds, false if it doesn't.
>   */
> -static int fmt_validate(char *template, char *user)
> +static bool fmt_validate(char *template, char *user)
>  {
> -	int valid = 1;
> -	int still_comparing = 1;
> +	bool valid = 1;
> +	bool still_comparing = 1;

These should be true, not 1.

julia

>  	char *template_ptr = template;
>  	char *user_ptr = user;
>
> @@ -516,10 +517,10 @@ static int fmt_validate(char *template, char *user)
>  			valid = compare_specifiers(&template_ptr, &user_ptr);
>  		} else {
>  			/* No more format specifiers in one or both strings. */
> -			still_comparing = 0;
> +			still_comparing = false;
>  			/* See if one has more specifiers than the other. */
>  			if (template_ptr || user_ptr)
> -				valid = 0;
> +				valid = false;
>  		}
>  	}
>  	return valid;
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/02ddf8f1b64cf7b893b462042c5c4a78116a645b.1489088501.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

[PATCH v3 0/2] staging: speakup: i18n.c: Refactor conditionals and change return type

[Narcisa Ana Maria Vasile]

Change return type from int to bool and refactor code for clarity.

---
Changes in v3:
    Replace numeric value '1' with boolean 'true'

Changes in v2:
    - Update documentation for compare_specifiers and fmt_validate
    - Make commit messages more clear
    - Rewrite condition without using '!'
    - Fit function call on single line

Narcisa Ana Maria Vasile (2):
    staging: speakup: i18n.c: Change return type from int to bool
    staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

    drivers/staging/speakup/i18n.c | 67 ++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 35 deletions(-)

    -- 
    1.9.1

[PATCH v3 1/2] staging: speakup: i18n.c: Change return type from int to bool

[Narcisa Ana Maria Vasile]

The possible return values (0 or 1) for compare_specifiers
and fmt_validate represent whether a condition holds or not, so
conceptually, they are booleans.

Update documentation for these two functions.

Change type of variable 'still_comparing' from int to bool too,
inside fmt_validate, because it is intended to hold truth values
as well.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index ac1ebea..a8326db 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -476,19 +476,20 @@ static char *find_specifier_end(char *input)
 /*
  * Function: compare_specifiers
  * Compare the format specifiers pointed to by *input1 and *input2.
- * Return 1 if they are the same, 0 otherwise.  Advance *input1 and *input2
- * so that they point to the character following the end of the specifier.
+ * Return true if they are the same, false otherwise.
+ * Advance *input1 and *input2 so that they point to the character following
+ * the end of the specifier.
  */
-static int compare_specifiers(char **input1, char **input2)
+static bool compare_specifiers(char **input1, char **input2)
 {
-	int same = 0;
+	bool same = false;
 	char *end1 = find_specifier_end(*input1);
 	char *end2 = find_specifier_end(*input2);
 	size_t length1 = end1 - *input1;
 	size_t length2 = end2 - *input2;
 
 	if ((length1 == length2) && !memcmp(*input1, *input2, length1))
-		same = 1;
+		same = true;
 
 	*input1 = end1;
 	*input2 = end2;
@@ -499,12 +500,12 @@ static int compare_specifiers(char **input1, char **input2)
  * Function: fmt_validate
  * Check that two format strings contain the same number of format specifiers,
  * and that the order of specifiers is the same in both strings.
- * Return 1 if the condition holds, 0 if it doesn't.
+ * Return true if the condition holds, false if it doesn't.
  */
-static int fmt_validate(char *template, char *user)
+static bool fmt_validate(char *template, char *user)
 {
-	int valid = 1;
-	int still_comparing = 1;
+	bool valid = true;
+	bool still_comparing = true;
 	char *template_ptr = template;
 	char *user_ptr = user;
 
@@ -516,10 +517,10 @@ static int fmt_validate(char *template, char *user)
 			valid = compare_specifiers(&template_ptr, &user_ptr);
 		} else {
 			/* No more format specifiers in one or both strings. */
-			still_comparing = 0;
+			still_comparing = false;
 			/* See if one has more specifiers than the other. */
 			if (template_ptr || user_ptr)
-				valid = 0;
+				valid = false;
 		}
 	}
 	return valid;
-- 
1.9.1

[PATCH v3 2/2] staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

[Narcisa Ana Maria Vasile]

Reduce the indentation level in spk_msg_set and remove
unnecessary return variable.

Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>
---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
index a8326db..7809867 100644
--- a/drivers/staging/speakup/i18n.c
+++ b/drivers/staging/speakup/i18n.c
@@ -541,34 +541,30 @@ static bool fmt_validate(char *template, char *user)
  */
 ssize_t spk_msg_set(enum msg_index_t index, char *text, size_t length)
 {
-	int rc = 0;
 	char *newstr = NULL;
 	unsigned long flags;
 
-	if ((index >= MSG_FIRST_INDEX) && (index < MSG_LAST_INDEX)) {
-		newstr = kmalloc(length + 1, GFP_KERNEL);
-		if (newstr) {
-			memcpy(newstr, text, length);
-			newstr[length] = '\0';
-			if (index >= MSG_FORMATTED_START &&
-			    index <= MSG_FORMATTED_END &&
-			    !fmt_validate(speakup_default_msgs[index],
-					  newstr)) {
-				kfree(newstr);
-				return -EINVAL;
-			}
-			spin_lock_irqsave(&speakup_info.spinlock, flags);
-			if (speakup_msgs[index] != speakup_default_msgs[index])
-				kfree(speakup_msgs[index]);
-			speakup_msgs[index] = newstr;
-			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
-		} else {
-			rc = -ENOMEM;
-		}
-	} else {
-		rc = -EINVAL;
+	if ((index < MSG_FIRST_INDEX) || (index >= MSG_LAST_INDEX))
+		return -EINVAL;
+
+	newstr = kmalloc(length + 1, GFP_KERNEL);
+	if (!newstr)
+		return -ENOMEM;
+
+	memcpy(newstr, text, length);
+	newstr[length] = '\0';
+	if (index >= MSG_FORMATTED_START &&
+	    index <= MSG_FORMATTED_END &&
+	    !fmt_validate(speakup_default_msgs[index], newstr)) {
+		kfree(newstr);
+		return -EINVAL;
 	}
-	return rc;
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	if (speakup_msgs[index] != speakup_default_msgs[index])
+		kfree(speakup_msgs[index]);
+	speakup_msgs[index] = newstr;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+	return 0;
 }
 
 /*
-- 
1.9.1

Re: [Outreachy kernel] [PATCH v3 2/2] staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

[Julia Lawall]

The subjject line is not correct.  There should not be .c.  In general,
you need to run git log --oneline on the file and see what are the common
subject lines for patches that touch this file.  There is no deterministic
algorithm for choosing the subject line, so you should not try to make up
the file name part of the subject line on your own.

On Fri, 10 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Reduce the indentation level in spk_msg_set and remove
> unnecessary return variable.
>
> Signed-off-by: Narcisa Ana Maria Vasile <narcisaanamaria12@gmail.com>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

This looks much nicer now :)

If you want to search for other cases like this you could try the
following semantic patch:

@disable neg_if@
expression e,E;
statement S;
@@

*if (e)
S
else { return -E; }

@disable neg_if@
expression e,E;
statement S;
identifier l;
@@

*if (e)
S
else { rc = -E; goto l; }

The disable neg_if prevents Coccinelle from considering the case where the
branches are flipped around.  Due to the *, this will give you something
that looks like a patch but in which the lines that contain the header of
a matching if have a - at the beginning.  It's just for information, not a
suggestion to remove the code.

julia


> ---
>  drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c
> index a8326db..7809867 100644
> --- a/drivers/staging/speakup/i18n.c
> +++ b/drivers/staging/speakup/i18n.c
> @@ -541,34 +541,30 @@ static bool fmt_validate(char *template, char *user)
>   */
>  ssize_t spk_msg_set(enum msg_index_t index, char *text, size_t length)
>  {
> -	int rc = 0;
>  	char *newstr = NULL;
>  	unsigned long flags;
>
> -	if ((index >= MSG_FIRST_INDEX) && (index < MSG_LAST_INDEX)) {
> -		newstr = kmalloc(length + 1, GFP_KERNEL);
> -		if (newstr) {
> -			memcpy(newstr, text, length);
> -			newstr[length] = '\0';
> -			if (index >= MSG_FORMATTED_START &&
> -			    index <= MSG_FORMATTED_END &&
> -			    !fmt_validate(speakup_default_msgs[index],
> -					  newstr)) {
> -				kfree(newstr);
> -				return -EINVAL;
> -			}
> -			spin_lock_irqsave(&speakup_info.spinlock, flags);
> -			if (speakup_msgs[index] != speakup_default_msgs[index])
> -				kfree(speakup_msgs[index]);
> -			speakup_msgs[index] = newstr;
> -			spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> -		} else {
> -			rc = -ENOMEM;
> -		}
> -	} else {
> -		rc = -EINVAL;
> +	if ((index < MSG_FIRST_INDEX) || (index >= MSG_LAST_INDEX))
> +		return -EINVAL;
> +
> +	newstr = kmalloc(length + 1, GFP_KERNEL);
> +	if (!newstr)
> +		return -ENOMEM;
> +
> +	memcpy(newstr, text, length);
> +	newstr[length] = '\0';
> +	if (index >= MSG_FORMATTED_START &&
> +	    index <= MSG_FORMATTED_END &&
> +	    !fmt_validate(speakup_default_msgs[index], newstr)) {
> +		kfree(newstr);
> +		return -EINVAL;
>  	}
> -	return rc;
> +	spin_lock_irqsave(&speakup_info.spinlock, flags);
> +	if (speakup_msgs[index] != speakup_default_msgs[index])
> +		kfree(speakup_msgs[index]);
> +	speakup_msgs[index] = newstr;
> +	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +	return 0;
>  }
>
>  /*
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/89a7e1067f4868fa5ef02ae94c32b6aff37b529e.1489099212.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v3 0/2] staging: speakup: i18n.c: Refactor conditionals and change return type

[Julia Lawall]

On Fri, 10 Mar 2017, Narcisa Ana Maria Vasile wrote:

> Change return type from int to bool and refactor code for clarity.
>
> ---
> Changes in v3:
>     Replace numeric value '1' with boolean 'true'
>
> Changes in v2:
>     - Update documentation for compare_specifiers and fmt_validate
>     - Make commit messages more clear
>     - Rewrite condition without using '!'
>     - Fit function call on single line
>
> Narcisa Ana Maria Vasile (2):
>     staging: speakup: i18n.c: Change return type from int to bool
>     staging: speakup: i18n.c: Refactor conditionals in spk_msg_set

Acked-by: Julia Lawall <julia.lawall@lip6.fr>


>
>     drivers/staging/speakup/i18n.c | 67 ++++++++++++++++++++----------------------
> 1 file changed, 32 insertions(+), 35 deletions(-)
>
>     --
>     1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1489099212.git.narcisaanamaria12%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>