staging: speakup: i18n.c: Refactor conditionals and change return type

PATCH Superseded Narcisa Ana Maria Vasile 2 participants 14 messages Mar 9, 2017 – Mar 10, 2017 Staging

Share & tools

Narcisa Ana Maria Vasile

author

2017-03-09 08:26 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/staging/speakup/i18n.c +8 -8

index 1a3e348..00ffa4e 100644
@@ -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

Julia Lawall

2017-03-09 08:58 UTC #2 lore raw

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

Quoted 15 lines · 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 <[email protected]>
---
 drivers/staging/speakup/i18n.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/staging/speakup/i18n.c +0 -0
index 1a3e348..00ffa4e 100644

drivers/staging/speakup/i18n.c static char *find_specifier_end(char *input) · line 481

Quoted 3 lines

drivers/staging/speakup/i18n.c +0 -0

@@ -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

Quoted 53 lines

drivers/staging/speakup/i18n.c +8 -9

-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 [email protected].
To post to this group, send email to [email protected].
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.

Narcisa Ana Maria Vasile

author

2017-03-09 08:26 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

drivers/staging/speakup/i18n.c +20 ~~-24~~

index 00ffa4e..a5b343c 100644
@@ -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

Julia Lawall

2017-03-09 08:55 UTC #4 lore raw

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

Quoted 34 lines · 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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

drivers/staging/speakup/i18n.c +0 ~~-15~~

index 00ffa4e..a5b343c 100644
@@ -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])

drivers/staging/speakup/i18n.c static bool fmt_validate(char *template, char *user) · line 547

Quoted 10 lines

drivers/staging/speakup/i18n.c +2 -8

-				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.

drivers/staging/speakup/i18n.c static bool fmt_validate(char *template, char *user) · line 557

Quoted 10 lines

drivers/staging/speakup/i18n.c +10 -0

+	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

Quoted 23 lines

drivers/staging/speakup/i18n.c +8 -2
+		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 [email protected].
To post to this group, send email to [email protected].
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.

Narcisa Ana Maria Vasile

author

2017-03-09 19:53 UTC #5 lore raw

v2 cover staging: speakup: i18n.c: Refactor conditionals and change return type

In reply to 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(-)

Signature · 1 line

1.9.1

Narcisa Ana Maria Vasile

author

2017-03-09 19:53 UTC #6 lore raw

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

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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/staging/speakup/i18n.c +12 ~~-11~~

index ac1ebea..522d719 100644
@@ -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;

Signature · 1 line

1.9.1

Julia Lawall

2017-03-09 20:43 UTC #7 lore raw

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

Quoted 49 lines · 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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/staging/speakup/i18n.c +6 -5

index ac1ebea..522d719 100644
@@ -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.

drivers/staging/speakup/i18n.c static int compare_specifiers(char **input1, char **input2) · line 508

Quoted 10 lines

drivers/staging/speakup/i18n.c +4 -4

- * 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

Quoted 26 lines

drivers/staging/speakup/i18n.c +2 -3
 	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 [email protected].
To post to this group, send email to [email protected].
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.

Narcisa Ana Maria Vasile

author

2017-03-09 19:53 UTC #8 lore raw

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

In reply to Narcisa Ana Maria Vasile

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

Signed-off-by Narcisa Ana Maria Vasile <[email protected]>

---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

drivers/staging/speakup/i18n.c +20 ~~-24~~

index 522d719..180dee3 100644
@@ -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;
 }
 
 /*

Signature · 1 line

1.9.1

Narcisa Ana Maria Vasile

author

2017-03-09 22:53 UTC #9 lore raw

v3 cover staging: speakup: i18n.c: Refactor conditionals and change return type

In reply to 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

Narcisa Ana Maria Vasile

author

2017-03-09 22:53 UTC #10 lore raw

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

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 <[email protected]>

---
 drivers/staging/speakup/i18n.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/staging/speakup/i18n.c +12 ~~-11~~

index ac1ebea..a8326db 100644
@@ -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;

Signature · 1 line

1.9.1

Narcisa Ana Maria Vasile

author

2017-03-09 22:53 UTC #11 lore raw

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

In reply to Narcisa Ana Maria Vasile

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

Signed-off-by Narcisa Ana Maria Vasile <[email protected]>

---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

drivers/staging/speakup/i18n.c +20 ~~-24~~

index a8326db..7809867 100644
@@ -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;
 }
 
 /*

Signature · 1 line

1.9.1

Julia Lawall

Acked-by on v3 2/2

2017-03-10 06:34 UTC #12 lore raw

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

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.

Quoted 4 lines · Narcisa Ana Maria Vasile

Reduce the indentation level in spk_msg_set and remove unnecessary return variable.
Signed-off-by Narcisa Ana Maria Vasile <[email protected]>

Acked-by Julia Lawall <[email protected]>

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

Quoted 73 lines

---
 drivers/staging/speakup/i18n.c | 44 +++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

drivers/staging/speakup/i18n.c +20 ~~-25~~

index a8326db..7809867 100644
@@ -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 [email protected].
To post to this group, send email to [email protected].
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.

Julia Lawall

Acked-by on the v3 series

2017-03-10 06:38 UTC #13 lore raw

In reply to Narcisa Ana Maria Vasile

Quoted 12 lines · 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

Quoted 3 lines

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 <[email protected]>

Quoted 14 lines

    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 [email protected]. To post to this group, send email to [email protected].
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.