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

Nette

If TRUE is zero, then yes! Otherwise, not really. It might sound nice to have operations independent of the actual values of booleans, but in practice it’s just messy, as you say. – Mailemailed

@RyanO'Hara I love it! Technically correct, but I can confirm that TRUE is not defined as 0 in this case. Man, that would be even weirder. . . – Nette

[opinion based] not, it is complete nonsense, but it can be used to please java-hipsters ;-) – During

For some reason, in my organization the coding convention enforcing to spell out the values.. Well, people are just not following though :) – Parterre

if(SomeFunction() == TRUE) has limited value, like int SomeFunction() returns 3 or more values and codes need to test if the return value was TRUE – Kazmirci

IMO: no. I have never seen any need to use formal boolean definitions, unless you want a language proof way to assign true (not always 1). In C 0 is false and anything else is true. int apples = 3; if(apples) {...} – Rettarettig

and the weird way that they made 'return' look like a function invocation What do you mean? Also, it s/b return (3 == value) ? 0 : 1; – Everybody

@Everybody even simpler is return 3 != value; – Rettarettig

@Everybody this thing is full of lines like 'return(42)' instead of 'return 42' The parenthesis seem unnecessary. To me it reads like someone is trying to call a function named return and pass a value of 42 – Nette

@KevinDTimm: A return statement does not require parentheses around the expression. The parentheses are not incorrect (it's just a parenthesized expression), but they're unnecessary and potentially obfuscating. return(42); looks very similar to a function call; return 42; clearly is a return statement. And a pre-C99 compiler will flag retrun 42; but is likely to compile retrun(42); without complaint, resulting in a link-time error. – Nancynandor

@RyanO'Hara has a point however; a less perverse and very likely scenario is where SomeFunction() returns some non-zero value other then whatever TRUE is defined as; in which case it will also fail. – Yaekoyael

@KeithThompson - For sure it does not, but for those of us who've used C for 30+ years there is nothing at all unusual about that syntax. Additionally, since it compiles but doesn't link the coder gets to learn about syntax errors :) – Everybody

The first line of your question true rather then TRUE, while elsewhere your refer to TRUE. If stdbool.h is included, true is defined and has type _Bool (with a typedef alias bool) and is a genuine boolean type. You may even get a warning if you attempt to compare it to an int or enum or whatever BOOLEAN is defined as. – Yaekoyael

@Yaekoyael - fixed. Thanks. It's TRUE everywhere. – Nette

Good, unfortunately @KeithThompson's excellent answer refers to your original text. It would be better perhaps to make it bool and true throughout. The problem with defining a boolean alias is that everyone does it perhaps slightly differently using different base types such as int, char or an enum for example, declared using macros or typedef, but often using the same or similar symbol names which makes using code from multiple sources a real pain when every vendor thought it was a good idea to define a boolean type. Not only is the idiom in question a bad idea, defining BOOLEAN is too – Yaekoyael

In the end this is a question of style, and stylistic issues are full of opinions, and everyone's entitled to their own opinion. Nevertheless, the opinion that if(SomeFunction() == TRUE) is good style is simply wrong. Some people will tell you that the explicit == TRUE is important for some reason or other. You can't convince them otherwise, so just ignore them. – Lindbom

@Clifford: If <stdbool.h> is not included, true could be anything. (Not relevant here, since the code uses TRUE.) – Nancynandor

@KeithThompson : Yes; that was rather my point; not only that but it would cause further problems when mixing with code that does use stdbool.h. – Yaekoyael

@SteveSummit: Seeing your name reminded me to cite the comp.lang.c FAQ in my answer. Thanks. – Nancynandor

@SteveSummit : No it is not just a matter of style, it is potentially the cause of some insidious bugs and maintenance issues. You should read the answers and other comments. – Yaekoyael

@Yaekoyael Don't worry, I read them all. (But are you sure you read all of mine? :-) ) – Lindbom

Even worse, defining _BOOLEAN causes undefined behavior. Identifiers starting with two underscores, or with an underscore and an uppercase letter, are reserved to the implementation. Rather than typedef enum _BOOLEAN { FALSE = 0, TRUE } BOOLEAN;, it could be just typedef enum { FALSE, TRUE } BOOLEAN; – Nancynandor

@SteveSummit:, re: "You can't convince them otherwise, so just ignore them." That is true, true, so very TRUE. :-) – Kazmirci

Since it appears not much reason to code this way, I am wondering if your next question is "Any reason for leaving code as-in like: if(function() == TRUE)?" which will have pros and cons. The reasons for the "con" apply to why this style of code should not have been used in the first place. – Kazmirci

@chux I'll probably go in and very carefully make sure that everything that returns a BOOLEAN returns either TRUE or FALSE and not a 3rd value, then change the call sites to if(function()) so that the next poor sap who has to work on this doesn't have to wonder WTF. It'll be easy to know it's right because of all of the unit tests that we . . . . hahaha just kidding! Of course there are no unit tests of any kind! – Nette

@Pete Baughman Agree: The trick is that if any function BOOLEAN f() returns something other than FALSE, TRUE (After all its only an enumerated type) you are S.O.L.. If sizeof(_Bool) != sizeof(BOOLEAN), or alignof(_Bool) != alignof(BOOLEAN) you get to lose even more sleep. I do not think the rank will be a concern, but you might want to check if both promote to int. And I hope no code uses BOOLEAN as a bit field. – Kazmirci

Most use that form because they like it. It's kinda like asking if you like tabbed indentation or spaced indentation, preference (Also, I don't need to look up return values) – Interchangeable

I'd first search for all occurences of BOOLEAN, TRUE and FALSE, vrify they are identically use like the standard type/macros and replace them if they are. This might be done by a simple search/replace or refactoring tool. Next phase is to modify the useless comparisons with TRUE/FALSE. Then simplify where the values are generated. All presuming you have the time and your bosses won't cancel the rework mid-term (all seen). – Anjanette

@ShaheAnsar : It is not as trivial as whitespace preferences; it has semantic implications from the definition of BOOLEAN, TRUE and FALSE which can cause bugs, maintenance, portability and interoperability issues. – Yaekoyael

@Yaekoyael I myself use it that way because I prefer it. A lot of people I know do it that way too, although yes, I do agree with you. – Interchangeable

I'm seeing the exact same pattern throughout example embedded code written by the chip manufacturer, and I really want to remove all of it in case it helps optimization, but I'm scared it does something non-obvious. if (IsUSBConfigured() != FALSE)?? – Nearly

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.

Nancynandor

I'd add that the (SomeFunction() == TRUE) style imposes an additional maintenance burden going forward, because every function incorporated into the system is required to provide return values adhering to the convention. There is great potential for insidious bugs to creep in here, especially if this convention later falls out of favor, but the old code continues to hang around in places. – Handmaid

Keith, John - This is the answer that I believed when I wrote the question, and I still believe this is right. I'm going to keep this open for a little while though in case some highly experienced embedded systems developer comes along and says "actually on the pic32 when using compiler version whatever . . . you need to watch out for . . ." – Nette

@PeteBaughman : I would suggest that the best answer is the one that works identically on any compiler and with any reasonable definition of BOOLEAN, TRUE and FALSE. An argument for doing this because it somehow works better in some specific compiler is a bad argument in any case. That said, I am an advocate of leaving at least 24 hours before accepting an answer to give all timezones a change to chip in. There really is no hurry. and Keith does not look like he's hungry for rep! – Yaekoyael

The use of BOOLEAN instead of _Bool or bool does throw in unnecessary complications. Good that you acknowledged implications if they are not synonyms. OP used them interchangeably - regrettably without a BOOLEAN, TRUE, FALSE definition. Still does not counter the conclusion of this fine answer. – Kazmirci

@chux Can you point out the mistake so I can fix it? It should all be BOOLEAN, TRUE, and FALSE. The definition that this project is using for BOOLEAN is given at the end of the question under "Additional Information." I'm open to suggestions to improve this. I fixed a true->TRUE typo about an hour ago. I don't see where I used BOOLEAN _Bool or bool interchangeably. – Nette

@chux: Thanks. I think you may have been looking at an earlier version of the question. It seems to be consistent now (unless I've missed something). – Nancynandor

@OP Comment referenced earlier version of post if(SomeFunction() == true). I do see the update with improved consistency: you might want to make consistent "SomeFunction returns true or false" --> "SomeFunction() returns TRUE or FALSE" – Kazmirci

Personally, I prefer the form: switch (SomeFunction()){ case TRUE: /* do stuff then */ break; default: break;}. Or sometimes res = SomeFunction(); while(res--){ /* do stuff */}, but the second form requires that TRUE == 1. – Calorimeter

@BrianMcFarland: (Yes, I'm replying to a comment from 2½ years ago.) Why? Do you always prefer switch to if? – Nancynandor

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

Yaekoyael

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.

Wellspoken

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

Iconolatry

Even such standards are partly suffering from wrong opinions. Just because there is a standard, it does not mean that it is reasonable or even correct. Disclaimer: I'm in the business (safety-related software for transportation, vulgo trains) for 20+ years. – Bisulfate

I understand and mostly agree, I have to respect those, but I do think many of these rules have a negative cost/benefit ratio :) never said this standard is perfect or reasonable :) – Iconolatry

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

Crus

Can you provide some evidence, for example by assembly code generated by the OP's compiler system? Otherwise I assume that a decent optimizing compiler will produce the same machine code from both versions. – Bisulfate

Search for it, "compare to zero" is the keyword – Crus

I have a firm background in assembly, therefore I claim that comparing to zero is as fast as comparing to non-zero, in nearly all instructions sets. That's why I asked you to provide some incidence for this specific case. General opinions and saying "likely" are "thin ice" to base decisions on. ;-) – Bisulfate

Try in arm 32bits compiler. int i = 0xffff; if (i == 0xffff) i++; if (i) i--;. First if requires 2 registers (one for i and other for 0xffff), it requires one ldr and one cmp for first comparison. The second if is just required one cmp rX, #0 instruction. Happy? – Crus

You cannot reason by this and have missed to provide the code. 1) Your i is an integer with an arbitrary value, not a boolean true like in the OP's question. 2) The target system is a PIC, not an ARM. – Bisulfate

:D, speechless. – Crus

I mean some evidence like this: Correct example on Compiler Explorer. To make things easier for you, it is compiled for an ARM target. Both version have the same number of instructions, there is no difference. – Bisulfate

godbolt.org/z/vM3xb9KMd here you are. – Crus

The fallacy in your example is that you have an integer (the base for enumeration types) with two explicitly defined values, but not a boolean, as the C standard defines it. Just because you call the type BOOLEAN and a non-zero value TRUE does not make it a boolean. After correcting your example (set TRUE to 1), your example also proves my claim. – Bisulfate

C doesn't have boolean! – Crus

See for example chapters 6.2.5, 5.2.4.2.1, 7.18, and many other findings of _Bool. Granted, it is a kind of integer, but with some specifics. Anyway, "true" and "false" are defined as 1 and 0, respectively, not an arbitrary non-zero and zero. It becomes clear, if for example you rename your BOOLEAN to ID, FALSE to THIS, and TRUE to THAT. The resulting code stays the same, but the semantics are different. – Bisulfate

Kind of extension, not standard. BTW: TRUE and FALSE in this case are aliases; your assumtion is not always right dude. And what I mentioned in my first comment is "compare to zero" is likely faster, so if the example I posted doesn't make sense to you I don't care. Let's end here, cuz IMO it won't lead to anywhere. – Crus

Right, let's stop it. You failed to prove your claim. – Bisulfate