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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/charconv:253
         {
-            if (!('0' <= *__p && *__p <= '9'))
+            if ('0' > *__p || *__p > '9')
                 break;
----------------
Mordante wrote:
> 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')`.
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.


================
Comment at: libcxx/include/fstream:754
         {
-            _LIBCPP_ASSERT ( !(__extbufnext_ == NULL && (__extbufend_ != __extbufnext_)), "underflow moving from NULL" );
+            _LIBCPP_ASSERT (__extbufnext_ != NULL || __extbufend_ == __extbufnext_, "underflow moving from NULL" );
             if (__extbufend_ != __extbufnext_)
----------------
ldionne wrote:
> philnik wrote:
> > ldionne wrote:
> > > Here too, I don't find the new formulation more readable.
> > 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.
> 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?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137804