2

I'm looking at some old code I inherited, and I really don't like the style at places. One of the things I really don't like the look of, is something like:

bool func() {
    bool ret = true;

    ret &= test1();
    homemade_assert_like_function(ret == true);
    ret &= test2();
    homemade_assert_like_function(ret == true);

    return ret;
}

Where I think the following looks much cleaner:

bool func() {
    homemade_assert_like_function(test1());
    homemade_assert_like_function(test2());

    return true; // just to keep the interface
}

However, the latter produces more assembler code. Not surprisingly, if I change the &= from the first example to = the result is the same as assert(test());, but still &= seems to be better

I use gcc5.4 for mips with -O2 for all three examples, but the result is similar if I use gcc for pc. If I compile with -std=c++14 (and a newer compiler) the three examples produce the same code.

Can anyone explain this to me? Is it just my test code that is really bad?

EDIT: I made a new code example, fixing the broken assert. It did have an interesting side effect. The size difference is much smaller now, indicating to me that it must be that the optimizer can do some magic in the &= case.

https://godbolt.org/z/Vk6uoY

As having been pointed out multiple times already, the example code is somewhat obfuscated. But it shows the same as I see on the real code - that changing nothing but removing the &= style of code, make the code base increase.

nikolaj
  • 180
  • 1
  • 10
  • 3
    Do you really think using `assert()` instead of using `&=` and **actually returning a value** is merely a choice of code **style**? – Andrew Henle Jan 09 '20 at 14:29
  • 4
    I think this is a classical mistake with the code completely being removed in non debug builds, en.cppreference.com: If NDEBUG is defined as a macro name at the point in the source code where is included, then assert does nothing. – gast128 Jan 09 '20 at 14:29
  • Your `assert` works reversed. – Jester Jan 09 '20 at 14:35
  • Different C code may result in different assembly output depending on the compiler and this is OK as long as the produced code behaves correctly. – Jabberwocky Jan 09 '20 at 14:35
  • @gast128 he has his own `assert` that is not removed in release builds – Jabberwocky Jan 09 '20 at 14:36
  • Then I forgot to mention that this does not use cassert. As I commented in the code examples, the implements its own assert. I could as have called it `wait_for_wdt_to_kill_me()` in the example. – nikolaj Jan 09 '20 at 14:37
  • 2
    Except your assert is broken because it's reversed. – Jester Jan 09 '20 at 14:39
  • @nikolaj: ok you changed the example since it was confusing. Further I am not sure if it is allowed to use the bitwise and operator on Boolean's. I think it must be ret = ret && test1(); – gast128 Jan 09 '20 at 14:41
  • 2
    @gast128 It's fine (but weird) to use bitwise operators on _Bool in C, since it counts as an integer type. – Lundin Jan 09 '20 at 14:43
  • 1
    @jester what do you mean by the `assert` being reversed ? As I understand it, it is not reversed. The `assert` does nothing if the argument is `true`. – chmike Jan 09 '20 at 14:44
  • Actually, the homemade assert implementation IS broken in this example. My bad, I just didn't want to bother you guys with all the logging and stuff happing in the one from the real code. But fixing it still does not change the amount of asm being produced. – nikolaj Jan 09 '20 at 14:48
  • 3
    But your version actually goes into an endless loop if the condition is true. Fixing it did change the amount of assembly when I checked. – Jester Jan 09 '20 at 14:48
  • @lundin: it can technically work but it's bad practice, see https://stackoverflow.com/questions/24542/using-bitwise-operators-for-booleans-in-c and https://stackoverflow.com/questions/48588556/bitiwise-exclusive-or-with-stdbool-h-bool-types-in-c-and-assignment – gast128 Jan 09 '20 at 15:00
  • The original function will return `false` in case any of the tests fail, so returning `true` in your example not only does not follow the api, but also changes it, as the return value will be true despite any of the tests fail. By the way, it passes the result of the first test to the first function, but the boolean and of the first test with the second to the second function, which is not what you suggest in your code. Your proposed code is not equivalent in any way to the original. – Luis Colorado Jan 12 '20 at 16:44

2 Answers2

3

This code is trying to use shortcut evaluation, but it is failing.
(the code is wrong)

First, let me show what it what it meant to do:

The code should have read:

bool func() {
    bool ret = true;

    ret = ret && test1();
    homemade_assert_like_function(ret == true);

    ret = ret && test2();
    homemade_assert_like_function(ret == true);

    return ret;
}

With the important part being that if ret is already false, indicating that the result has failed, it will not even try to evaluate the second part of the expression! (test1() or test2()).

If function test() has any side-effects, it will not be run once the ret flag indicates failure.

This is useful for going through a long multi-step process (test1() ...testN()) and not doing any work once a single step has failed.

Now, what is this code actually doing, and why?

ret starts as true, and as soon as one of the functions fails, ret becomes false, and gets stuck at false (so long as the operation is always &=.

But, unlike what I wrote above, every function, test1(), test2(), testN(), does still get run (including side effects), even if a prior test already failed!

This is no different than if they had just written the code as:

bool func() {
    bool ret = true;

    ret = test1();
    homemade_assert_like_function(ret == true);
    ret = test2();
    homemade_assert_like_function(ret == true);

    return ret;
}

The big difference from the code you suggest is that your code always returns true. How can you presume that every test succeeded? The code should be allowed to return false when a test fails. You dropped that feature from your version of the code.

abelenky
  • 63,815
  • 23
  • 109
  • 159
  • `true && ....` is obviously never short-circuit evaluated, so no, there's no rationale, just a confused programmer who wrote the original code. _However_, using `&&` introduces sequence points, which could in theory block the compiler from generating the most effective code possible. – Lundin Jan 09 '20 at 15:10
  • 1
    @Lundin: the shortcircuiting happens on `test2()` (and presumably `test3()`... 'testN()`). The syntax is kept on `test1()` for consistency in the code, even though `test1()` will always get evaluated. – abelenky Jan 09 '20 at 15:12
  • @Lundin: I think this answer is talking about the NDEBUG case, where the asserts *don't* bail out of the function on false. That should be stated explicitly, and made into a bigger deal to make it clear when it applies. (Also, the OP has a hand-rolled assert that might never be disabled, IDK.) – Peter Cordes Jan 09 '20 at 15:12
  • @abelenky: *How can you presume that every test succeeded?* The OP and Lundin don't; they only assumes that `return` can't be reached because `homemade_assert_like_function` will bail out if that's not the case, and never return. If that's not what `homemade_assert_like_function`, then it's very badly misnamed and isn't actually an assert. (Or you're talking about the NDEBUG case, assuming `homemade_assert_like_function` supports that.) – Peter Cordes Jan 09 '20 at 15:16
  • @Peter, thats a pretty big assumption. Traditional asserts are disabled in `RELEASE` / `NDEBUG` code, and only active in DEBUG code. You do NOT want `RELEASE` and `DEBUG` code to behave differently – abelenky Jan 09 '20 at 15:17
  • *Traditional asserts are disabled in RELEASE / NDEBUG code.* So **say that in your answer** ; that's the central justification for your argument. But I'd still argue that the use of `assert()` could mean the program has no way to handle the `return false` case, and certainly not a *tested* way exactly because it's not reachable in debug mode. Also, the OP's `homemade_assert` doesn't (currently) have an NDEBUG; it's *always* an early-out. (Actually an infinite loop that waits for the watchdog timer, except they wrote it backwards.) – Peter Cordes Jan 09 '20 at 15:54
  • 1
    @abelenky: Yes, I think the first version of the code you wrote there, is actually what was intended originally. The only reason you are wrong about you second point is that the code I posted really is that bad - yes it always return true (if it returns) and therefore the `&=` can be removed, with the only side effect that the code size grew. – nikolaj Jan 09 '20 at 16:07
-1

I think it is your test code that's unnecessarily complex. If you disassemble just the function itself, you'll see that cosmetic changes to the function leads to close to identical code. In fact they lead to slightly faster code.

I got rid of all the pointless obfuscation and wrote a readable version:

#include <stdbool.h>

bool func (void) {
    assert(test1());
    assert(test2());
    return true;
}

11 instructions on gcc x86-64 9.2 -O2. But your original version gives 15 instructions. Changing the function types to bool leads to slightly different code, but still the clean version is more efficient.

https://godbolt.org/z/4Cmr_H

The important part here is however to write the most readable code. Writing weird stuff like bool ret = true; ret &= test1(); is just plain senseless.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • 1
    Your `func` never returns `false` when a test fails. The original code returned `false` if a test fails, and assertions are disabled (such as RELEASE code). – abelenky Jan 09 '20 at 15:15
  • 1
    @abelenky : also the original code is functional not the same since besides the different return statement, the assert will fire twice in the first example if test1 fails. If I make them functional the same with storing the result to different Boolean variables there is still a (small) difference in assembly output when using msvc but the same when using gcc. – gast128 Jan 09 '20 at 15:21
  • 1
    @abelenky Look at the OP's godbolt code. It's not standard `assert` and besides neither you nor the OP defined `NDEBUG`. – Lundin Jan 09 '20 at 15:21
  • Like with @abelenky's answer, you should probably go through *your* reasoning explicitly to explain why this is equivalent: `return` is unreachable because of `assert` if either test returns false. So the function always returns true or aborts. Once you're done debugging (so asserts never fire), it just plain always returns true. And/or that the OP's hand-rolled assert doesn't even support an `NDEBUG` so it *always* works to prevent `return true` from being reached if either test fails. – Peter Cordes Jan 09 '20 at 15:48
  • 1
    @Lundin: Good answer :) Yes, declerating the functions as prototypes is far cleaner and ensures that the function are not inlined. I completely agree that the repeated `&=` is pointless (that's was why I was removing it from the code), but in the end, also in the real code, I see that size of the asm code increse when I use `assert(test());` vs the `&=` style. – nikolaj Jan 09 '20 at 15:54
  • 2
    @Peter Cordes: It was a bad Idea that I had called it assert() from the start. It was just the name the function had en the code I inherited. It does not support NDEBUG, and yes, the function can only return true or die and wait for the wdt. It is bad code, and that is why I was trying to clean it up when I stumpled over that curiosity I asked about. – nikolaj Jan 09 '20 at 15:56
  • @nikolaj: Put that in question. Also, fix your `homemade_assert_like_function` to infinite loop if the condition is *false*, or name it differently ( `stop_if_true` ) and fix the callers to pass `!ret`. – Peter Cordes Jan 09 '20 at 15:59