[libc++][clang-tidy] Enable readability-simplify-boolean-expr

[Eugene.Zelenko]

libcxx/.clang-tidy

31

Please keep alphabet order.

[philnik]

• Rebased
• Address coment

[ldionne]

I am not against this, but it's not clear to me that this really increases readability that much, since 50% of the changes suggested by the tool are not improvements IMO.

libcxx/include/charconv

253

I don't find the new one to be more readable. The old one read more naturally as "if *__p is not between '0' and '9'". The new one isn't as clear.

libcxx/include/fstream

755–756

Here too, I don't find the new formulation more readable.

libcxx/include/locale

4015–4016

Same.

[philnik]

In D137804#3939919, @ldionne wrote:

I am not against this, but it's not clear to me that this really increases readability that much, since 50% of the changes suggested by the tool are not improvements IMO.

I'm actually quite surprised by that, given that I find the second one you commented on basically unreadable in the current state. Maybe I'm just exceptionally bad at reading negations though.

libcxx/include/charconv

253

Here I don't care super much, but I'd read the new version as "if *__p is outside 0 to 9". IMO "outside" is a bit nicer than "not inside", which I'd mentally convert to "outside".

libcxx/include/fstream

755–756

In this case I find the new version much more readable. Instead of "it's not the case that __extbufnext_ is null and __extbufend_ is not equal to __extbufnext_", it reads to me as "__extbufnext_ is not null or __extbufend_ is equal to __extbufnext_". In case of the second one I instantly know that the the iterator is non-null or range is empty; IOW check that we don't dereference a nullptr. To get to that conclusion I actually have to DeMorgan the original version in my head.

[ldionne]

libcxx/include/charconv

253

What I like about the previous formulation is that '0' <= *__p && *__p <= '9' immediately reads as 0 <= p <= 9 in my head, I have no need to even think about it. I can't say the same for '0' > *__p || *__p > '9'.

In general though -- if we enable this clang-tidy check, what do we do when the tool suggests something that we don't agree with? Can we turn it off for this warning? Tools are nice but I want us to keep in mind that they're only tools meant to help us, the humans, do a better job. If a tool suggests something we don't think is better, we should be happy to turn it off for that instance.

libcxx/include/fstream

755–756

When I read an assertion, I generally read it as assert(!bad-condition-that-shouldnt-happen). In this case, the left hand side made that really clear. With your explanation I do agree that the right hand side also makes sense, but I would then change the code to this instead:

if (__extbufend_ != __extbufnext_) {
    _LIBCPP_ASSERT(__extbufnext_ != NULL, "underflow would be calling memmove on a nullptr");
    _VSTD::memmove(__extbuf_, __extbufnext_, __extbufend_ - __extbufnext_);
}

This makes it clearer that we're specifically trying to guard against this nullptr condition in memmove. WDYT?

[Mordante]

I too am not convinced by this clang-tidy check. I agree it makes the expression simpler, but it seems to make certain cases harder to understand since the "complex" form is more intuitive. I am happy to see we only have a few places where clang-tidy wants to improve our code!

libcxx/include/charconv

253

+1 I find this quite confusing. I would write the original or if (*__p < '0' || *__p > '9').

[philnik]

Address comments

libcxx/include/charconv

253

IMO @Mordante's version is the most readable. @ldionne any objections to that?

Re. disabling a check: You can use // NOLINT, // NOLINTNEXTLINE and // NOLINTBEGIN + // NOLINTEND to ignore clang-tidy. Optionally, you can also use //NOLINT*(clang-tidy-check-to-ignore) to disable a specific check (IMO this should be the default). For more details see https://clang.llvm.org/extra/clang-tidy/index.html#suppressing-undesired-diagnostics.

libcxx/include/fstream

755–756

Sounds good.

[ldionne]

libcxx/include/charconv

253

@Mordante 's suggestion is fine with me.

[njames93]

For what it's worth, the clang tidy check can be configured to disable transformations based off DeMorgan's law.

libcxx/.clang-tidy

Checks: >
   readability-duplicate-include,
   readability-identifier-naming,
   readability-function-cognitive-complexity,
   readability-function-size,
   readability-misplaced-array-index,
   readability-redundant-control-flow,
   readability-redundant-function-ptr-dereference,
   readability-redundant-preprocessor,
+  readability-simplify-boolean-expr,
   readability-simplify-subscript-expr,
   readability-uniqueptr-delete-release,
 
 CheckOptions:
   - key:   readability-function-cognitive-complexity.Threshold
     value: 143 # TODO: bring that number down
   - key:   readability-function-size.LineThreshold
     value: 194 # TODO: bring that number down
   - key:   readability-identifier-naming.GetConfigPerFile
     value: false
   - key:   readability-identifier-naming.ParameterCase

 # modernize-use-nullptr,
 # portability-restrict-system-includes,
 # readability-function-cognitive-complexity,
 # readability-implicit-bool-conversion,
 # readability-isolate-declaration,
 # readability-redundant-access-specifiers,
 # readability-redundant-declaration,
 # readability-redundant-member-init,
-# readability-simplify-boolean-expr,
+#

libcxx/include/__format/unicode.h

_LIBCPP_HIDE_FROM_ABI constexpr bool __at_extended_grapheme_cluster_break(
 
   // *** Do not break within emoji flag sequences ***
 
   // That is, do not break between regional indicator (RI) symbols if there
   // is an odd number of RI characters before the break point.
 
   if (__prev == __property::__Regional_Indicator && __next == __property::__Regional_Indicator) { // GB12 + GB13
     __ri_break_allowed = !__ri_break_allowed;
-    if (__ri_break_allowed)
-      return true;
-
-    return false;
+    return __ri_break_allowed;
   }
 
   // *** Otherwise, break everywhere. ***
   return true; // GB999
 }
 
 /// Helper class to extract an extended grapheme cluster from a Unicode character range.
 ///

libcxx/include/__locale

const char_type* scan_is (mask __m, const char_type* __low, const char_type* __high) const
                 break;
         return __low;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     const char_type* scan_not(mask __m, const char_type* __low, const char_type* __high) const
     {
         for (; __low != __high; ++__low)
-            if (!(isascii(*__low) && (__tab_[static_cast<int>(*__low)] & __m)))
+            if (!isascii(*__low) || !(__tab_[static_cast<int>(*__low)] & __m))
                 break;
         return __low;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     char_type toupper(char_type __c) const
     {
         return do_toupper(__c);

libcxx/include/charconv

struct _LIBCPP_HIDDEN __traits : __traits_base<_Tp>
     static _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI char const*
     __read(char const* __p, char const* __ep, type& __a, type& __b)
     {
         type __cprod[digits];
         int __j = digits - 1;
         int __i = digits;
         do
         {
-            if (!('0' <= *__p && *__p <= '9'))
+            if (*__p < '0' || *__p > '9')
                 break;
             __cprod[--__i] = *__p++ - '0';
         } while (__p != __ep && __i != 0);
 
         __a = __inner_product(__cprod + __i + 1, __cprod + __j, __pow() + 1,
                               __cprod[__i]);
         if (__mul_overflowed(__cprod[__j], __pow()[__j - __i], __b))
             --__p;

libcxx/include/fstream

if (this->gptr() == this->egptr())
             {
                 this->setg(this->eback(),
                            this->eback() + __unget_sz,
                            this->eback() + __unget_sz + __nmemb);
                 __c = traits_type::to_int_type(*this->gptr());
             }
         }
         else
         {
-            _LIBCPP_ASSERT ( !(__extbufnext_ == NULL && (__extbufend_ != __extbufnext_)), "underflow moving from NULL" );
-            if (__extbufend_ != __extbufnext_)
+            if (__extbufend_ != __extbufnext_) {
+                _LIBCPP_ASSERT(__extbufnext_ != nullptr, "underflow moving from NULL" );
                 _VSTD::memmove(__extbuf_, __extbufnext_, __extbufend_ - __extbufnext_);
+            }
             __extbufnext_ = __extbuf_ + (__extbufend_ - __extbufnext_);
             __extbufend_ = __extbuf_ + (__extbuf_ == __extbuf_min_ ? sizeof(__extbuf_min_) : __ebs_);
             size_t __nmemb = _VSTD::min(static_cast<size_t>(__ibs_ - __unget_sz),
                                  static_cast<size_t>(__extbufend_ - __extbufnext_));
             codecvt_base::result __r;
             __st_last_ = __st_;
             size_t __nr = fread((void*) const_cast<char *>(__extbufnext_), 1, __nmemb, __file_);
             if (__nr != 0)

libcxx/include/locale

if (this->gptr() == this->egptr())
             {
                 this->setg(this->eback(),
                            this->eback() + __unget_sz,
                            this->eback() + __unget_sz + __nmemb);
                 __c = *this->gptr();
             }
         }
         else
         {
-             _LIBCPP_ASSERT(!(__extbufnext_ == NULL && (__extbufend_ != __extbufnext_)), "underflow moving from NULL" );
-             if (__extbufend_ != __extbufnext_)
+             if (__extbufend_ != __extbufnext_) {
+                _LIBCPP_ASSERT(__extbufnext_ != NULL, "underflow moving from NULL" );
                 _VSTD::memmove(__extbuf_, __extbufnext_, __extbufend_ - __extbufnext_);
+             }
             __extbufnext_ = __extbuf_ + (__extbufend_ - __extbufnext_);
             __extbufend_ = __extbuf_ + (__extbuf_ == __extbuf_min_ ? sizeof(__extbuf_min_) : __ebs_);
             streamsize __nmemb = _VSTD::min(static_cast<streamsize>(this->egptr() - this->eback() - __unget_sz),
                                  static_cast<streamsize>(__extbufend_ - __extbufnext_));
             codecvt_base::result __r;
             // FIXME: Do we ever need to restore the state here?
             //state_type __svs = __st_;
             streamsize __nr = __bufptr_->sgetn(const_cast<char*>(__extbufnext_), __nmemb);