Any reason for if(function() == TRUE) in C

Question

The Question: Does doing if(SomeFunction() == TRUE) instead of doing if(SomeFunction()) protect against some type of coding error? I'm trying to understand if this is protecting from some hidden land-mine, or if it's the result of someone writing code who didn't quite understand how expressions are evaluated. I understand that if done right, both of these things evaluate the same. Just like if(value == 42) and if(42 == value) evaluate the same - still, some prefer the 2nd version because it produces a compiler error if someone typo's the == and writes = instead.

Background: I've inherited some embedded software that was written 4 or 5 years ago by people who don't work here anymore. I'm in the middle of some refactoring to get rid of multi-hundred line functions and global variables and all that jazz, so this thing is readable and we can maintain it going forward. The code is c for a pic microprocessor. This may or may not be relevant. The code has all sorts of weird stuff in it that screams "didn't know what they were doing" but there's a particular pattern (anti-pattern?) in here that I'm trying to understand whether or not there's a good reason for

The Pattern: There are a lot of if statements in here that take the form

if(SomeFunction() == TRUE){
  . . .
}

Where SomeFunction() is defined as

BOOLEAN SomeFunction(void){
  . . .
  if(value == 3)
    return(FALSE);
  else
    return(TRUE);
}

Let's ignore the weird way that SomeFunction returns TRUE or FALSE from the body of an if statement, and the weird way that they made 'return' look like a function invocation.

It seems like this breaks the normal values that c considers 'true' and 'false' Like, they really want to make sure the value returned is equal to whatever is defined as TRUE. It's almost like they're making three states - TRUE, FALSE, and 'something else' And they don't want the 'if' statement to be taken if 'something else' is returned.

My gut feeling is that this is a weird anti-pattern but I want to give these guys the benefit of the doubt. For example I recognize that if(31 == variable) looks a little strange but it's written that way so if you typo the == you don't accidently assign 31 to variable. Were the guys that wrote this protecting against a similar problem, or is this just nonsense.

Additional Info

• When I wrote this question, I was under the impression that stdbool was not available, but I see now that it's provided by the IDE, just not used in this project. This tilts me more towards "No good reason for doing this."
• It looks like BOOLEAN is defined as typedef enum _BOOLEAN { FALSE = 0, TRUE } BOOLEAN;
• The development environment in question here is MPLAB 8.6

Answer 1

Is there a good reason for doing if(SomeFunction() == true) instead of doing if(SomeFunction())

No.

If SomeFunction() returns a result of type _Bool, then the equality comparison should be reliable (assuming that the evaluation of the result did not involve undefined behavior). But the use of TRUE rather than true, and the type name BOOLEAN rather than _Bool or bool, suggest that the result is not an actual _Bool (available only in C99 or later), but some ad-hoc Boolean-like type -- perhaps an alias for int.

A value of any scalar type can be used as a condition in an if statement. If the value is equal to zero, the condition is false; otherwise, the condition is true. If TRUE is defined as 1, and SomeFunction() returns, say, 3, then the test will fail.

Writing

if (SomeFunction()) { /* ... */ }

is simpler, clearer, and more likely to behave correctly.

Note, for example, that the isdigit() et al functions declared in <ctype.h> do not just return 0 or 1; if the argument is a digit, isdigit() can (and does) return any non-zero value. Code that uses it is expected to handle it correctly -- by not comparing it for equality to 1, to true, or to TRUE.

Having said that, there might be a valid reason to compare something for equality to TRUE -- if it matters whether the result is equal to TRUE or has some other non-zero value. But in that case, using the names BOOLEAN and TRUE is misleading. The whole point of a Boolean type is that values are either true or false; there's no "maybe", and if there happen to be different representations of truth, you shouldn't care which one you have.

The guideline I try to follow is:

Never compare a logically Boolean value for equality or inequality to true or false (or 0, 1, FALSE, TRUE). Just test the value directly, with a ! operator if you want to invert the test. (A "logically Boolean" value either is of type _Bool, or is intended to distinguish between truth and falsehood with no additional information. The latter can be necessary if _Bool is not available.) Comparison to false can be safe, but there's no reason to do it; comparing the value directly is still clearer.

And if someone tells you that

if (SomeFunction() == true)

is better than

if (SomeFunction())

just ask them why

if ((SomeFunction() == true) == true)

isn't even better.

See also section 9 of the comp.lang.c FAQ. Its emphasis on pre-C99 solutions is perhaps a bit dated, but it's still valid.

UPDATE: The question asks about a function that returns the value TRUE of type BOOLEAN, defined something like this:

typedef enum { FALSE, TRUE } BOOLEAN;

Such definitions were useful in pre-1999 C, but C99 added the predefined Boolean type _Bool and the header <stdbool.h>, which defines macros bool, false, and true. My current advice: Use <stdbool.h> unless there's a serious concern that your code might need to be used with an implementation that doesn't support it. If that's a concern, you can use

typedef enum { false, true } bool;

or

typedef int bool;
#define false 0
#define true 1

(I prefer the first.) This isn't 100% compatible with the C99 definitions, but it will work correctly if you use it sensibly.

Answer 2

Since in C any non-zero value is considered true and only zero false you should never compare to one specific TRUE macro definition in any event. It is unnecessarily specific. The form:

if( fn() )

is the simplest form, but if you do prefer to compare to a specific value, then only compare to FALSE thus:

if( fn() != FALSE )  // Safer than '== TRUE', but entirely unnecessary

which will work for all reasonable definitions of FALSE and also if fn() is not BOOLEAN. But it remains totally unnecessary.

Personally for easier debugging I'd prefer:

 BOOLEAN x = fn() ;
 if( x )

As well as being able to observe the return value in your debugger before entering or skipping the conditional block, you have the opportunity to name x something self documenting and specific to the context, which the function name might not reflect. In maintenance you are more likely to maintain a variable name than correct a comment (or many comments). In addition x is then available to use elsewhere rather then calling fn() multiple times (which if it has side effects or state may not return the same value).

Another problem with user defined boolean types and values is that the definitions may not be consistent throughout - especially if you use third-party code whose authors also thought it a good idea to define their own using the same symbol names as yours. If the names differ (such as BOOL, BOOLEAN or OS_BOOL for example), when your code interfaces to this third-party code, you then have to decide whose boolean type should be used in any particular circumstance, and the names of TRUE and FALSE are likely to clash with redefinition warnings or errors.

A better approach would be to update the code to use stdbool.h and the real boolean type bool (an alias for the built in _Bool in C99) which can have only two values true and false. This will still not protect you from the case where fn() is not a bool function and returns an integer other then zero or one, but there is then the chance that the compiler will issue a type mismatch warning. One of the best things you can do to refactor legacy code in and case is to set the warning level high and investigate and fix all the warnings (and not just by liberal casting!).

Answer 3

It is probably very late and I am not 100% sure about PICs, but in AVRs some registers are available for application use and they offer very fast read and write speed.

The compiler place the Boolean variables there first and then use the SRAM. In order to stop the compiler from doing so, usually a 8-bit variable is used instead of Boolean so that the compiler use the available register for variables used the most in the application. By doing so the application run faster but you should not use Booleans or only when they are used a lot. The only option is to use uint8_t/int8_t or you may use a self made type to act as a Boolean while being a uint8_t/int_t.

For the return section, I have always done this in embedded systems. You return one value from the function and that value can represent successful operation. Other values are used to show errors or for debugging. This way one variable is used for everything and since you would want to avoid using Boolean, you can use the 8-bit variable with full potential.

Overall I would like to emphasize something here, the embedded folks care about speed and memory a lot more than software engineers. I have worked with both folks, embedded system engineers generate a garbage looking code but it runs fast and consume very little memory. One of the reason that the code looks bad/ugly is because the compilers are not as smart as the typical compilers used by software engineers and sometimes special tweaks (which may look stupid) results in a more efficient assembly code. On the other hand, software engineers spend more time on readability and may not care about the efficiency of the code as much because the compiler will do most of the job.

If the old embedded engineers were experienced and/or smart, they have probably done all of these for a good reason. Specially if they know the hardware very well.

I hope this help for the rest of the code you are going through.

Answer 4

Yes, some consider it is safer to write more explicit code, and this is one of the many rules C programmers in sensitive industrial or embedded software ( trains, planes . . . ) have to respect , read them all here in the ANSSI guidelines "RULES FOR SECURE C LANGUAGE SOFTWARE DEVELOPMENT" :

https://www.ssi.gouv.fr/uploads/2022/04/anssi-guide-rules_for_secure_c_language_software_development-v1.4.pdf

you will possibly find many of these rules are silly or useless, but this is a standard . . .

Answer 5

Compare to zero likely faster than non-zero.

if (SomeFunction()) // compare to zero

if (SomeFunction() == TRUE) // compare to non-zero (assume TRUE != 0)

In practise, if (SomeFunction()) forces you to choose the meaningful name for the function which then makes the code more readable. E.g

if (hasFinished()) {
   // yes, finished, do something
}

instead of

if (GetStatus() == TRUE) {
    // GetStatus returns TRUE if finished, do something
}

So, unless you have to comply with any standard, try if (SomeFunction()).