Why is TMath::IsNaN returning Int_t?

Hi there,

just a (stupid?) question: why does TMath::IsNaN() return Int_t instead of Bool_t?

Reason for the question: I ran clang-tidy over my code and it showed

myfile.cpp:93:13: warning: implicit cast 'Int_t' -> bool [readability-implicit-bool-cast]
        if (TMath::IsNaN(score)) {
                                != 0

In general, I like this checker very much, but obviously I don’t want to add a !=0 check to IsNaN!

At first it looks like a legacy code and sloppy programming to me. IsNaN, depending on configuration (my guess) is either calling std::isnan, which has the return type bool, or ROOT::Math::Internal::IsNan, which requires R__FAST_MATH and after pretending being smart and fast and whatever … converts an essentially bool expression into this Int_t for the reason that is unknown.
Maybe, they first were using isnan from C headers (which seems to be a macro there, since C does not have overloading and in my implementation, for example, resolves to something like

__header_always_inline int __inline_isnanf(float __x) {
   // hehe, note, in C the result of != has type **int**
    return __x != __x; 

) and then somebody “modernized” the code by just prepending std:: and changing <math.h> to <cmath>.
Saying that - if you look at how this IsNan used, you’ll find cases there the type is, indeed, important - calling Max(isNan(), somethingElseNotBool) for example.

Thanks for the comprehensive review, @tpochep :slight_smile:

Still - this is hmm, awkward. Shall we fix TComplex to not use Max() but ||, and make IsNaN return a bool? @moneta - what do you think?

I think historically TMath::IsNaN was returning an int following the C insan implementation that returns an integer.
I agree we should change to Bool_t. It is not a backward-incompatible change, but it is clear in same cases users might get a compiler error, like when using it in template functions


That’s now https://github.com/root-project/root/pull/1179

Merged! Thanks, @behrenhoff !

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.