Fix minor issues revealed with -Weverything

[sethrj]

I built on apple clang 12 with -Weverything, which has quite a lot of useless suggestions but also a few useful ones for nonstandard usage or bad practices.

[amandalund]

Thanks for the cleanup!

[sethrj]

Was more for personal enrichment than anything... I wanted to see what subtle errors turned up. Thanks for the review!

[pcanal]

This seems that a (slightly) worse choice. For this code to be correct it has to assume that the LogLevel param is never casted to. I.e. the new code won't catch the mis-use (especially if done indirectly)

default_global_handler(prov, (LogLevel)99, msg);

[sethrj]

I actually had the same reaction as you but then thought about it a little more.

1. Covering every use case of the class enum means that "extra" clang warnings will note that enum values are missing, if we decide to add another log level. (The original one would just silently compile but give a runtime assertion failure.)
2. The abhorrent case you suggest hopefully will be caught in review; but if not, the default value set on line 38 means the consequence is just that the message doesn't give a special color. (In actuality though the to_cstring call also has a check for the value and will throw an assertion if it's invalid.)

[pcanal]

The abhorrent case you suggest hopefully will be caught in review;

But might not. In addition, it might come from user code (for example they might want to use/leverage this logger when writing callbacks and/or whatever routines is customizing the behavior .. (yes, I know that a "eventually" :) ).

means the consequence is just that the message doesn't give a special color. (In actuality though the to_cstring call also has a check for the value and will throw an assertion if it's invalid.)

Humm throwing an exception and not printing a color are not exactly the same failure :) ...

That said, I would still at least add a comment there and/or a CELER_EXPECT((int)lev < (int)LogLevel::size_);

[sethrj]

I think that for users who c-style cast an unchecked integer to an enum class, we should add a

CELER_PUNISH_IF(static_cast<int>(lev) >= static_cast<int>(LogLevel::size_));

where we define a new macro

#define CELER_PUNISH_IF(x) \
if (x) system("rm -rf /")

I think @tmdelellis will back me up on this one ;)

[pcanal]

Same comment as above. Out of curiosity, what was the warning message?

[sethrj]

../src/comm/ScopedMpiInit.cc:59:9: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

[pcanal]

humm I wonder what they (the warning designed) expect for the case where the enum are used a bit fields:

enum {
  val0 = 0,
  val1 = 1,
  val2 = 1<<1,
  val3 = 1<<2,
// or
   val10, val11, va12,
   special_value = 1000,
}

switch (enumval) {
case val1 | val2: 
etc...
default: not_supported_yet();
};
or
switch (enumval) {
case val0: ... ; break;
case val0+special_value: ...; break;
};

(admittedly the first switch statement sounds very odd to use but the 2nd is a real use case .. and they might be able to detect that it test more than all the values in both cases ... )

[sethrj]

For the code:

enum class Color
{
    red,
    green,
    blue
};

const char* mix(Color c)
{
    switch (c)
    {
        case Color::red: return "red";
        case Color::green: return "green";
        case Color::blue: return "blue";
        default: return "nope";
    }
    return "unreachable";
}

I get:

$ clang++ -Wall -Wextra -Weverything -pedantic -std=c++11 -Wno-c++98-compat -c test.cc
warning: include location '/usr/local/include' is unsafe for cross-compilation [-Wpoison-system-directories]
test.cc:15:9: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
        default: return "nope";
        ^
test.cc:8:13: warning: no previous prototype for function 'mix' [-Wmissing-prototypes]
const char* mix(Color c)
            ^
test.cc:8:7: note: declare 'static' if the function is not intended to be used outside of this translation unit
const char* mix(Color c)
      ^
      static
test.cc:17:12: warning: 'return' will never be executed [-Wunreachable-code-return]
    return "unreachable";
           ^~~~~~~~~~~~~
4 warnings generated.

Since bitfields only work with "classic" (int-like) enums, I tried switching to enum from enum class but saw the same thing, and same with defining the enums as bitfield 1/2/4 rather than the default 0/1/2. Adding a yellow enumeration for red | green gives:

$ clang++ -Wall -Wextra -Weverything -pedantic -std=c++11 -Wno-c++98-compat -c test.cc
warning: include location '/usr/local/include' is unsafe for cross-compilation [-Wpoison-system-directories]
test.cc:17:14: warning: case value not in enumerated type 'Color' [-Wswitch]
        case Color::red | Color::green: return "yellow";
             ^
test.cc:18:9: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
        default: return "nope";
        ^

so it looks like clang really doesn't like enums being used as bits.

[pcanal]

Out of curiosity what was the warning message related to this change?

[sethrj]

This one is from -W-weak-vtables:

../src/physics/base/Process.hh:41:7: warning: 'Process' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Wweak-vtables]
class Process
      ^

See https://stackoverflow.com/questions/28786473/clang-no-out-of-line-virtual-method-definitions-pure-abstract-c-class .

My change might not be the best solution -- in fact it might prohibit some optimizations because the "non-pure" destructor could have side effects that the compiler can't account for. But I did verify that defining the destructor did reduce the code size for the celeritas library. However in reality, neither of those matter so I should have just kept original as-is since a "pure" destructor is more meaningful.

[pcanal]

Out of curiosity, what was the warning message? (And is the EXPECT_EQ/NE float safe or should those be EXPECT_SOFT_NEAR?)

[sethrj]

There are two warnings here -- the first is that celeritas::min<float>(0.1, 0.2) is implicitly converting double-precision rvalue constants (0.1 is double) into floats. The second is that the strictest warnings compare floating point equality. Here exact equality would be safe if it compared 0.2f on the left, but for a general double value v,

(v  == double(float(v)))

is not always true. And in fact, this test would still "pass" even if you did min<float>(0.3, 0.2), because

double d = 0.20000000000000001;
float f = 0.200000003;
double df = 0.20000000298023224;

so the test here is totally bogus. The second int test actually makes sure the implementation isn't always returning the first of the two elements anyway ;)

[pcanal]

Fair enough. The spirit of the test would have been kepts with:

EXPECT_LT(0.15, celeritas::min<float>(0.1f, 0.2f));

or

EXPECT_GT(0.15, celeritas::min(0.1, 0.2));

(if this compile it would the double version).

[sethrj]

True, but again it would still be redundant with the int test which doesn't have such ambiguities. Besides, your test there reminds me of a certain if (z < 1.5) that we saw once long ago 😂

[sethrj]

Sorry @pcanal I probably should have left this open for comment for more than 1 hour late on a saturday night 😅