[Asterisk-code-review] Clang: Fix some more tautological-compare warnings. (asterisk[13])

[Diederik de Groot]

Diederik de Groot has posted comments on this change.

Change subject: Clang: Fix some more tautological-compare warnings.
......................................................................


Patch Set 4:

(8 comments)

https://gerrit.asterisk.org/#/c/160/4/channels/chan_skinny.c
File channels/chan_skinny.c:

Line 7548: 			eventmessage = letohl(req->e);
         : 			if (eventmessage < 0) {
> I don't understand how this is any different from what was here before.
It isn't, (that's the point). The only difference is that the clang compiler will not complain about this. 

On little endian machine letohl(_X) be defined as (_X), making the comparison.

if ((req->e) < 0) {
req->e being an unsigned int, would always be true -> "tautological comparison" Warning generated. 

On big endian machine there actually is a swap happening, which might require the check to take place.

I did not want to change req->e to int, because i could not oversee the consequences.

The only difference is that eventmessage has now been cast to an int instead of to an unsigned int. Which makes the comparison "allowed", noting more.


https://gerrit.asterisk.org/#/c/160/4/main/security_events.c
File main/security_events.c:

Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> Clang apparently considers any enum that is defined with *no* negative valu
¶4 specifies that the implementation is free to choose as "base type" any particular type that is "compatible with char, a signed integer type or an unsigned integer type", as long as it can represent all the elements of the enum

Most compilers go for the unsigned int as far as i know (gcc as well i think). Other compiler just don't warn about it (gcc -ansi -pedantic should as well). This warning can be benefitial in other circumstances, just not this one.

If you want to make force the/any enum to be a signed integer, you would have to give the enum('s) an ERROR_VALUE entry with -1, while you are at it, you might as well add a MAX and MIN as well (which i think would be preferred over the change i made). (There might be an attribute you can add to the enum, but i don't know which).


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> What about at run-time though?  If ast_json_integer_get can return a negati
It's a compiler warning, it is not preventing anything at runtime. If the enum is an unsigned int, then -2 would become the 4,294,967,295 - 2 = 4,294,967,293, that's all

Best solution, force the enum to be signed by adding the actually negative value that you guys want to verify and remove my patch. I am absolutely for that ! I did not make that change because i cannot oversee the broad scope and or impact. Removing this "useless" check was the easier route. 

If a discussion like this was the result of making that change, i am even happier, just so we can come to a resolution, one way or the other.


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> Unless Clang also has some sort of compiler warning/error to prevent assign
I would warn about any of these:
enum ast_security_event_type XX= -1;
enum ast_security_event_type XX= 15;
enum ast_security_event_type XX = AST_FRAME_VOICE;

If that is what you mean. Setting a value outside of the enum range, and or using another ENUM even if it has a value within the range of the enum.

Using a negative value in an enum, is absolutely fine, and it will automatically change the type of the enum from unsigned int to signed int.


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> Why?
It compiles, it just warns you that you may be doing something wrong here. It will compile, run and work never the less. 

Clang is trying to be helpfull, preventing you from assigning:
 - values outside the range of the enum
 - enum-names which do not belong to this enum
But also doing useless "negative" checks against an enum which was declared as an unsigned int (less cpu cycles spent checking stuff that's cannot be true). 

And yes it can be a little pedantic at times, but it is for a good cause.


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> ast_json_integer_get is a wrapper around libjansson's json_integer_value(),
Several ways out:
- fix the wrapper to return valid values
- If you want to check for negative numbers, assign it to an int first, check and then assign it to the enum. 
- Add negative numbers to the enum, if the wrapper is allowed to report an error that way
- Add an &error argument to the wrapper so it can report errors that way
- Create a macro to check the validity of an assignment to an enum (checking the range / bounds, and if it's a sparse enum the actual existence), this macro can be a pass-through when not in devmode.

I don't think the discussion above is about this particular issue, but more about enum's in general


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> Its not that silly.  If an enum is defined with *no* negative values a vari
Done


Line 424: 	RAII_VAR(struct ast_str *, str, NULL, ast_free);
        : 	struct ast_json *event_type_json;
        : 	enum ast_security_event_type event_type;
        : 
        : 	event_type_json = ast_json_object_get(json, "SecurityEvent");
        : 	event_type = ast_json_integer_get(event_type_json);
        : 
        : 	ast_assert(event_type < AST_SECURITY_EVENT_NUM_TYPES);
> I disagree with this change and the one in res_security_log.c. An enum type
The standard is not clear about this. Either optimization is ok: the implementation is free to choose as "base type" any particular type that is "compatible with char, a signed integer type or an unsigned integer type", as long as it can represent all the elements of the enum.

I am absolutely fine with changing the enum, and restoring the check if that is the route to go. I would love to see that change made general and standard for all enum's. If all enums withing asterisk would follow that new rule. It could even result in code reduction, but implementing a couple of enum setter/getter macro's that would automatically check the bounds, if you would like to got there. At the moment there are several methods checking enums bounds all over the place. I think chan_sip is the only one using a macro for this.


-- 
To view, visit https://gerrit.asterisk.org/160
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ief23ef68916192b9b72dabe702b543ecfeca0b62
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Diederik de Groot <dkgroot at talon.nl>
Gerrit-Reviewer: Diederik de Groot <dkgroot at talon.nl>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes