[libc++] Prevent tautological comparisons

[smeenai]

Summary

Tip-of-tree clang produces -Wtautological-constant-compare for
tautological constant comparisons, and these pieces of code will trigger
that diagnostic when int and long have the same size (for example,
when compiling for a 32-bit target, or for Windows 64-bit).

I personally find the diagnostic to be pretty unhelpful when dealing
with templates, since my change is essentially manually writing out the
code the compiler would have (or at least should have) produced anyway.
An alternative would be to just disable the diagnostic around these
regions instead of manually avoiding the comparisons.

[smeenai]

Ping.

Like I said, not a huge fan of this change, but also not a huge fan of running into clang diagnostics (especially since we build with -Werror downstream).

[mclow.lists]

I dislike this change fairly strongly.
I would much rather pursue a clang-based solution (since clang is being unhelpful here)
Don't know if we can get one, though.

[lebedev.ri]

That is my diagnostic, so i guess this is the time to reply :)

In D39149#910825, @mclow.lists wrote:

I dislike this change fairly strongly.
I would much rather pursue a clang-based solution (since clang is being unhelpful here)
Don't know if we can get one, though.

As i see it, there are several options:

1. Disable that warning in libc++ cmakelists. Be careful though, i think it might disable the entire 'tautological comparison' diagnostic family.
2. Use preprocessor pragmas to disable the diagnostic for the selected code, rL315882. Brittle, ugly, effective, has the least impact. <- Best?
3. In -Wtautological-constant-compare, ignore any comparisons that compare with std::numeric_limits. Not a fan of this solution. <- Worst?
4. Disable that diagnostic by default in clang. Also, not really a fan of this solution. I implemented it because it would have caught a real bug in my code in rather shorter time, see mail.
5. The essential problem, i *think* is much like the problem with unreachable code diagnostic, CppCon 2017: Titus Winters “Hands-On With Abseil”. The check does not know/care whether the comparison is tautological only with the current Fundamental type sizes, or always. Perhaps this is the proper heading?
6. ???

[mclow.lists]

If we have to go down this road, I'd prefer the approach used in http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp?r1=315874&r2=315873&pathrev=315874

(which is your solution #2)

[smeenai]

In D39149#910825, @mclow.lists wrote:

I dislike this change fairly strongly.

Agreed. Would you be happier with just disabling the diagnostics around the problematic parts?

In D39149#910845, @lebedev.ri wrote:

1. Disable that warning in libc++ cmakelists. Be careful though, i think it might disable the entire 'tautological comparison' diagnostic family.

Note that one of the tautological comparisons is in a header, so it affects all libc++ clients and not just libc++ itself.

2. Use preprocessor pragmas to disable the diagnostic for the selected code, rL315882. Brittle, ugly, effective, has the least impact. <- Best?

I was considering doing this instead. It also seems ugly in its own way, but possibly less ugly than this patch.

5. The essential problem, i *think* is much like the problem with unreachable code diagnostic, CppCon 2017: Titus Winters “Hands-On With Abseil”. The check does not know/care whether the comparison is tautological only with the current Fundamental type sizes, or always. Perhaps this is the proper heading?

Yeah, exactly. If the warning could only fire when the comparison was *always* going to be tautological, that would avoid any issues like this, but I'm not sure how best to go about that.

[smeenai]

In D39149#910858, @mclow.lists wrote:

If we have to go down this road, I'd prefer the approach used in http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp?r1=315874&r2=315873&pathrev=315874

(which is your solution #2)

All right, I'll change this patch accordingly.

[bcain]

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)

...

3. In -Wtautological-constant-compare, ignore any comparisons that compare with std::numeric_limits. Not a fan of this solution. <- Worst?

...

Gee, #3 is the worst? That one seems the most appealing to me. I think I could understand it if you had reservations about ignoring comparisons against {CHAR,SHRT,INT,LONG}_{MIN,MAX} but std::numeric_limits is well-qualified and everything.

Can you help me understand why you don't prefer it?

Also, is there any way that the warning could somehow be smart enough to know what sizeof(int) and sizeof(long) are?

As an aside, how widely has this new clang behavior been tested? Is it possible that this new warning behavior might get rolled back if a big/important enough codebase triggers it?

[lebedev.ri]

In D39149#910891, @bcain wrote:

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)

...

3. In -Wtautological-constant-compare, ignore any comparisons that compare with std::numeric_limits. Not a fan of this solution. <- Worst?

...

Gee, #3 is the worst? That one seems the most appealing to me. I think I could understand it if you had reservations about ignoring comparisons against {CHAR,SHRT,INT,LONG}_{MIN,MAX} but std::numeric_limits is well-qualified and everything.

Can you help me understand why you don't prefer it?

That my initial bug was not caught by gcc either, even though -Wtype-limits was enabled.
Because for whatever reason gcc basically does not issue diagnostics for templated functions.
Even though the tautologically-compared variable was *not* dependent on the template parameters in any way.
See for yourself, i think this is *really* bad: https://godbolt.org/g/zkq3UD

So all-size-fits-all things like that are just asking to be done wrong.
I fail to see how ignore any comparisons that compare with std::numeric_limits would not be the same pitfall, unfortunately.

Also, is there any way that the warning could somehow be smart enough to know what sizeof(int) and sizeof(long) are?

I'm not sure i follow. It sure does know that, else it would not possible to diagnose anything.
Or were you talking about

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)
As i see it, there are several options:
...

5. The essential problem, i *think* is much like the problem with unreachable code diagnostic, CppCon 2017: Titus Winters “Hands-On With Abseil”. The check does not know/care whether the comparison is tautological only with the current Fundamental type sizes, or always. Perhaps this is the proper heading?

?

As an aside, how widely has this new clang behavior been tested? Is it possible that this new warning behavior might get rolled back if a big/important enough codebase triggers it?

Are you talking about false-positives* or about real true-positive warnings?
So far, to the best of my knowledge, there were no reports of false-positives*.
If a false-positive is reported, and it is not obvious how to fix it, i suppose it might be reverted.
True-positives are obviously not the justification for the revert, but a validation of the validity of the diagnostic.
The cases like this one are troublesome. If it is possible to reduce a case that obviously should not warn, then the diagnostic should be adjusted not to warn.

• The fact that under *different* circumstances the comparison is not tautological is not a false-positive.

[smeenai]

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

[smeenai]

In D39149#910931, @lebedev.ri wrote:

• The fact that under *different* circumstances the comparison is not tautological is not a false-positive.

It's not technically a false positive, but my suspicion is that it'll make the warning a lot less useful in a lot of cases.

[bcain]

In D39149#910931, @lebedev.ri wrote:

In D39149#910891, @bcain wrote:

In D39149#910845, @lebedev.ri wrote:

That is my diagnostic, so i guess this is the time to reply :)

...

3. In -Wtautological-constant-compare, ignore any comparisons that compare with std::numeric_limits. Not a fan of this solution. <- Worst?

...

Gee, #3 is the worst? That one seems the most appealing to me. I think I could understand it if you had reservations about ignoring comparisons against {CHAR,SHRT,INT,LONG}_{MIN,MAX} but std::numeric_limits is well-qualified and everything.

Can you help me understand why you don't prefer it?

That my initial bug was not caught by gcc either, even though -Wtype-limits was enabled.

...

Ok, yes, that makes sense.

Also, is there any way that the warning could somehow be smart enough to know what sizeof(int) and sizeof(long) are?

I'm not sure i follow. It sure does know that, else it would not possible to diagnose anything.

Yes, sorry, I had the logic reversed in my head.

...

As an aside, how widely has this new clang behavior been tested? Is it possible that this new warning behavior might get rolled back if a big/important enough codebase triggers it?

Are you talking about false-positives* or about real true-positive warnings?
So far, to the best of my knowledge, there were no reports of false-positives*.
If a false-positive is reported, and it is not obvious how to fix it, i suppose it might be reverted.
True-positives are obviously not the justification for the revert, but a validation of the validity of the diagnostic.
The cases like this one are troublesome. If it is possible to reduce a case that obviously should not warn, then the diagnostic should be adjusted not to warn.

• The fact that under *different* circumstances the comparison is not tautological is not a false-positive.

Isn't this a case that it obviously should not warn?

What about another approach: (1) could we consider Shoaib's suggestion to keep the exhaustive check with this name and create a new one that's limited (and can take the current one's place in the enabled-by-Wall) or (2) could we create range check functions that have the warning disabled?

Option #2 is limited to benefiting only llvm or libcxx code. But if we narrow our focus to just the llvm/libcxx code for the time being, we should have a common idiom for resolving this problem. It's likely to show up elsewhere too. The fact that we were considering ifdefs here versus the pragmas in the earlier commit is concerning.

[rsmith]

In D39149#910936, @smeenai wrote:

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

This sounds like a very promising direction. (That is: if the types of the values being compared are different, but are of the same size, then suppress the warning or move it to a different warning group that's not part of -Wtautological-compare.) That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

[lebedev.ri]

In D39149#911024, @rsmith wrote:

In D39149#910936, @smeenai wrote:

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

This sounds like a very promising direction. (

That is: if the types of the values being compared are different, but are of the same size, then suppress the warning or move it to a different warning group that's not part of -Wtautological-compare.

Ok, now that makes at least some sense :)

) That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

[smeenai]

In D39149#911024, @rsmith wrote:

In D39149#910936, @smeenai wrote:

I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.

This sounds like a very promising direction. (That is: if the types of the values being compared are different, but are of the same size, then suppress the warning or move it to a different warning group that's not part of -Wtautological-compare.) That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

Yeah, the type comparison implementation was what I was thinking of originally, though I wasn't sure about the edge cases.

When you say "always warning if the constant side is an integer literal", do you mean if it's a straight-up integer literal, or if it's an expression which evaluates to an integer literal at compile time? For example, would there be a difference if you were comparing to numeric_limits<int>::max() vs. INT_MAX?

[lebedev.ri]

In D39149#910825, @mclow.lists wrote:

I dislike this change fairly strongly.
I would much rather pursue a clang-based solution (since clang is being unhelpful here)
Don't know if we can get one, though.

@smeenai
D39462 *should* render this (D39149) differential unnecessary, *please* try it.

[smeenai]

I confirmed that these warnings go away with D39462 applied, and they reappear if I manually specify -Wmaybe-tautological-constant-compare. Thank you!

[bcain]

In D39149#912260, @smeenai wrote:

I confirmed that these warnings go away with D39462 applied, and they reappear if I manually specify -Wmaybe-tautological-constant-compare. Thank you!

Let's resurrect these changes since D39462 was not the right short-term approach.

[mclow.lists]

In D39149#958378, @bcain wrote:

Let's resurrect these changes since D39462 was not the right short-term approach.

Let's not. IMHO, these changes are:

• a bad idea in principle - clang should not be warning here, and
• a bad idea in execution - we don't want to be using __SIZEOF_INT__ and __SIZEOF_LONG__

I'm fine with the realization that "D39462 was not the right short-term approach", but that doesn't make this the right solution.

[smeenai]

I put up D41368 to just suppress the warnings instead.

include/istream

#endif  // _LIBCPP_NO_EXCEPTIONS
         sentry __s(*this);
         if (__s)
         {
             typedef istreambuf_iterator<char_type, traits_type> _Ip;
             typedef num_get<char_type, _Ip> _Fp;
             ios_base::iostate __err = ios_base::goodbit;
             long __temp;
             use_facet<_Fp>(this->getloc()).get(_Ip(*this), _Ip(), *this, __err, __temp);
+#if __SIZEOF_INT__ != __SIZEOF_LONG__
             if (__temp < numeric_limits<int>::min())
             {
                 __err |= ios_base::failbit;
                 __n = numeric_limits<int>::min();
             }
             else if (__temp > numeric_limits<int>::max())
             {
                 __err |= ios_base::failbit;
                 __n = numeric_limits<int>::max();
             }
             else
                 __n = static_cast<int>(__temp);
+#else
+            __n = static_cast<int>(__temp);
+#endif
             this->setstate(__err);
         }
 #ifndef _LIBCPP_NO_EXCEPTIONS
     }
     catch (...)
     {
         this->__set_badbit_and_consider_rethrow();
     }

src/string.cpp

 // string
 template<>
 inline
 int
 as_integer(const string& func, const string& s, size_t* idx, int base )
 {
     // Use long as no Standard string to integer exists.
     long r = as_integer_helper<long>( func, s, idx, base, strtol );
+#if __SIZEOF_INT__ != __SIZEOF_LONG__
     if (r < numeric_limits<int>::min() || numeric_limits<int>::max() < r)
         throw_from_string_out_of_range(func);
+#endif
     return static_cast<int>(r);
 }
 
 template<>
 inline
 long
 as_integer(const string& func, const string& s, size_t* idx, int base )
 {

 // wstring
 template<>
 inline
 int
 as_integer( const string& func, const wstring& s, size_t* idx, int base )
 {
     // Use long as no Stantard string to integer exists.
     long r = as_integer_helper<long>( func, s, idx, base, wcstol );
+#if __SIZEOF_INT__ != __SIZEOF_LONG__
     if (r < numeric_limits<int>::min() || numeric_limits<int>::max() < r)
         throw_from_string_out_of_range(func);
+#endif
     return static_cast<int>(r);
 }
 
 template<>
 inline
 long
 as_integer( const string& func, const wstring& s, size_t* idx, int base )
 {