[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

[Jeffrey Sorensen via Phabricator via cfe-commits]

sorenj added a comment.

So, I ran this check against the cxx directory of llvm, it fired 5 times so let's look at the context and disucss:

There are two identical spots in locale.cpp, the first is around line 2717

  uint16_t t = static_cast<uint16_t>(
           0xD800
         | ((((wc & 0x1F0000) >> 16) - 1) << 6)
         |   ((wc & 0x00FC00) >> 10));
   

the fact that a signed value is being right shifted here surprises me, but looking earlier there's a branch for wc < 0x010000 so we are safe here against wc being 0. So this is a false diagnostic. Still, wc is a uint32_t, it's the 0x1f0000 that converts it to signed. Probably should be 0x1f0000u? Will still false-alarm on this code though unless you use - 1u to make the whole thing unsigned end to end.

the third is in memory.cc

  void*
  align(size_t alignment, size_t size, void*& ptr, size_t& space)
  {
      void* r = nullptr;
      if (size <= space)
      {
          char* p1 = static_cast<char*>(ptr);
          char* p2 = reinterpret_cast<char*>(reinterpret_cast<size_t>(p1 + (alignment - 1)) & -alignment);

here it doesn't make sense for alignment to be zero, and the & -alignment will be zero anyway, so false alarm although some check for alignment > 0 might be an improvement

The last two are in valarray.cpp lines 35 and 43, I've copied a large excerpt here

  void
  gslice::__init(size_t __start)
  {
      valarray<size_t> __indices(__size_.size());
      size_t __k = __size_.size() != 0;
      for (size_t __i = 0; __i < __size_.size(); ++__i)
          __k *= __size_[__i];
      __1d_.resize(__k);
      if (__1d_.size())
      {
          __k = 0;
          __1d_[__k] = __start;
          while (true)
          {
              size_t __i = __indices.size() - 1;
              while (true)
              {
                  if (++__indices[__i] < __size_[__i])
                  {
                      ++__k;
                      __1d_[__k] = __1d_[__k-1] + __stride_[__i];
                      for (size_t __j = __i + 1; __j != __indices.size(); ++__j)
                          __1d_[__k] -= __stride_[__j] * (__size_[__j] - 1);
                      break;
                  }
                  else
                          

with some looking I can see that `__indices.size()` has to be non-zero. It's less clear to me that `size_[__j]` is strictly positive here and there should probably be some guard against underflow there.

That's a firing rate of about 5/12K lines of code, but I wonder if I were to send patches for these 3 files that silence the warning I wonder how many would be approved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71607