Build warnings on OpenBSD.

[vext01]

If you build on OpenBSD using clang, there are a lot of build warnings.

Most of them are like:

arith.c:503:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]

which are a result of functions catching a "this shouldn't happen" case with assert(false). The assertion would only be hit if assertions are enabled (building without -DNDEBUG).

Would it be better to do something like this?

diff --git a/arith.c b/arith.c
index 9e724f0b..f4be2330 100644
--- a/arith.c
+++ b/arith.c
@@ -20,6 +20,7 @@
 #include "arith.h"
 #include <assert.h>
 #include <errno.h>
+#include <err.h>
 #include <float.h>
 #include <limits.h>
 #include <locale.h>
@@ -498,7 +499,7 @@ long do_long_comparison(atokentype_T ttype, long v1, long v2)
         case TT_EXCLEQUAL:
             return v1 != v2;
         default:
-            assert(false);
+            errx(EXIT_FAILURE, "%s: unreachable", __func__);
     }
 }

Asides from that, there's also:

editing.c:597:14: warning: expression result unused [-Wunused-value]
        INIT(case_func, 0);
             ^~~~~~~~~
./../common.h:52:39: note: expanded from macro 'INIT'
# define INIT(x, dummy_initial_value) x

Thanks

[magicant]

The configure script by default does not enable any compiler warnings so that the release build goes in peace. If you added non-default warning options, is it not your responsibility to look after any false positives produced by them?

[vext01]

I didn't add any non-default warnings. This is the system clang on OpenBSD-current:

OpenBSD clang version 13.0.0
Target: amd64-unknown-openbsd7.4
Thread model: posix
InstalledDir: /usr/bin

Configured like this:

CC=clang CPPFLAGS="-std=c99 -I/usr/local/include" LDFLAGS="-L/usr/local/lib" ./configure

(-std=c99 is required to build)

so that the release build goes in peace

I think clang is correct to warn.

Because I didn't pass --debug to configure you get #define NDEBUG 1 in config.h and then all the assertions are removed.

From the assert.h manual:

The assert() macro may be removed at compile time with the cc(1) option -DNDEBUG.

So the code from the above diff looks like:

long do_long_comparison(atokentype_T ttype, long v1, long v2)
{
    switch (ttype) {
        case TT_LESS:
            return v1 < v2;
        case TT_LESSEQUAL:
            return v1 <= v2;
        case TT_GREATER:
            return v1 > v2;
        case TT_GREATEREQUAL:
            return v1 >= v2;
        case TT_EQUALEQUAL:
            return v1 == v2;
        case TT_EXCLEQUAL:
            return v1 != v2;
        default:
    }
}

clang is warning us that if we execute the default arm of the switch there is no explicit return value.

I actually have no idea what happens in this case. It could even be UB?

[magicant]

clang is warning us that if we execute the default arm of the switch there is no explicit return value.

I actually have no idea what happens in this case. It could even be UB?

You don't have to consider what happens if the default arm is taken, because the default arm is never taken, and that's why I say it is a false positive.

[vext01]

You don't have to consider what happens if the default arm is taken, because the default arm is never taken.

Right. Probably clang can't figure that out due to the compilation model of C.

If it were my decision I'd still change it. For two reasons:

• If later you were to add more constants to atokentype_T and forget to update all the switches that use it, then one of the defaults could become reachable (sure, you'd still get the assertion failure for debug builds, but personally I'd prefer a crash always).
• To silence the default-enabled compiler warnings for users. As you 've seen, I didn't turn on any extra warnings to get these messages.

If you still disagree, please feel free to close this. Thanks :)

[vext01]

I'll add a third reason I think we should fix this:

• The deluge of false positives may hide some true positives.

@magicant Would you mind if I did a PR for this? It shouldn't be much work.

[magicant]

Thank you for the proposal, but I'd like to seek to find a way to discipline the analyzer so that it doesn't make false positives without sacrificing the release build's optimality.

Please understand that any warnings you ever see are "opinionated" because:

• the compiler/linter/analyzer developers chose to regard that particular condition as suspicious and worth implementing a detector, and
• the compiler/linter/analyzer developers/packagers chose to enable that detection by default, and you chose to leave it enabled (if not explicitly enabled it).

None of these is my decision.

It is impossible to address every possible "I see a warning with my setup of this particular version/option/configuration of a particular compiler/linter/analyzer/etc. and I think this should be fixed" thing, so I only fix real bugs, not patterns that seem to be bugs to someone.

[vext01]

so that it doesn't make false positives without sacrificing the release build's optimality.

Would replacing the asserts with aborts (or similar) impact release builds if they are not executed? I wouldn't have thought so.

[magicant]

In the release build, assert(false) expands to nothing. Then smart compilers translate:

int fn(void) {
  switch (value) {
    case FOO: return 1;
    case BAR: return 2;
    default: assert(false);
  }
}

into:

int fn(void) {
  if (value == FOO) return 1; else return 2;
}

But this optimization cannot be applied if you enable the assertion or replace it with abort (or anything that is not a no-op).

The redundant code would also make the binary larger (though I don't know if it would be a significant difference).

[vext01]

OK. I won't push further. Thanks for considering it.