threads / patch / 44447

patchsequencer: shut up clang warning

Subject: [PATCH] sequencer: shut up clang warning

## tl;dr

3 messages between Nov 9, 2016 and Nov 9, 2016. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Johannes Schindelin· Nov 9, 2016, 13:56 UTC · lore

When comparing a value of type `enum todo_command` with a value that is outside the defined enum constants, clang greets the developer with this warning:

	comparison of constant 2 with expression of type
	'const enum todo_command' is always true

While this is arguably true *iff* the value was never cast from a free-form int, we should keep the cautious code in place.

To shut up clang, we simply introduce an otherwise pointless enum constant and compare against that.

Noticed by Torsten Bögershausen.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/enum-warning-v1
Fetch-It-Via: git fetch https://github.com/dscho/git enum-warning-v1

	Peff, if you would like me to include more of your commit message,
	please let me know.

	I will adjust the sequencer-i patches to keep the TODO_INVALID
	constant.

 sequencer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Show changes to sequencer.c +3 −2

diff --git a/sequencer.c b/sequencer.c
index 5fd75f3..f80e9c0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -619,7 +619,8 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
 
 enum todo_command {
 	TODO_PICK = 0,
-	TODO_REVERT
+	TODO_REVERT,
+	TODO_INVALID
 };
 
 static const char *todo_command_strings[] = {
@@ -629,7 +630,7 @@ static const char *todo_command_strings[] = {
 
 static const char *command_to_string(const enum todo_command command)
 {
-	if (command < ARRAY_SIZE(todo_command_strings))
+	if (command < TODO_INVALID)
 		return todo_command_strings[command];
 	die("Unknown command: %d", command);
 }

base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e

-- 
2.10.1.583.g721a9e0

Jeff King· Nov 9, 2016, 15:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] sequencer: shut up clang warning

On Wed, Nov 09, 2016 at 02:56:25PM +0100, Johannes Schindelin wrote:

Show 12 quoted lines

> When comparing a value of type `enum todo_command` with a value that is
> outside the defined enum constants, clang greets the developer with this
> warning:
> 
> 	comparison of constant 2 with expression of type
> 	'const enum todo_command' is always true
> 
> While this is arguably true *iff* the value was never cast from a
> free-form int, we should keep the cautious code in place.
> 
> To shut up clang, we simply introduce an otherwise pointless enum constant
> and compare against that.

This does silence the warning.

I slightly prefer mine because:

  1. It does not carry an implicit requirement that TODO_INVALID remain
     the final enum value.

  2. It also protects the range check against a negative enum value.

But this is code that is getting changed later by you anyway, and I do not care that strongly.

-Peff

Jakub Narębski· Nov 9, 2016, 22:28 UTC · re: Johannes Schindelin · lore

Re: [PATCH] sequencer: shut up clang warning

W dniu 09.11.2016 o 14:56, Johannes Schindelin pisze:

Show 17 quoted lines

> When comparing a value of type `enum todo_command` with a value that is
> outside the defined enum constants, clang greets the developer with this
> warning:
> 
> 	comparison of constant 2 with expression of type
> 	'const enum todo_command' is always true
> 
> While this is arguably true *iff* the value was never cast from a
> free-form int, we should keep the cautious code in place.
> 
> To shut up clang, we simply introduce an otherwise pointless enum constant
> and compare against that.
> 
> Noticed by Torsten Bögershausen.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Show 15 quoted lines

>  sequencer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5fd75f3..f80e9c0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -619,7 +619,8 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
>  
>  enum todo_command {
>  	TODO_PICK = 0,
> -	TODO_REVERT
> +	TODO_REVERT,
> +	TODO_INVALID
>  };

Why not name it TODO_N, or N_TODO, or something like that?

-- 
Jakub Narębski