[libcxx-commits] [PATCH] D137804: [libc++][clang-tidy] Enable readability-simplify-boolean-expr

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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!



================
Comment at: libcxx/include/charconv:253
         {
-            if (!('0' <= *__p && *__p <= '9'))
+            if ('0' > *__p || *__p > '9')
                 break;
----------------
ldionne wrote:
> philnik wrote:
> > ldionne wrote:
> > > 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.
> > 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".
> 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.
+1 I find this quite confusing. I would write the original or `if (*__p < '0' || *__p > '9')`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137804/new/

https://reviews.llvm.org/D137804