[PATCH] D39263: [support] remove tautological comparison in Support/Windows/Path.inc

[Zachary Turner via llvm-commits]

Up to you, but if you split it then i would prefer the semantic change
happens immediately after. It’s useful enough that I don’t want it to
shelved and subsequently forgotten about
On Wed, Oct 25, 2017 at 11:28 AM Bob Haarman via Phabricator <
reviews at reviews.llvm.org> wrote:

> inglorion added a comment.
>
> @zturner do you want me to make the semantic changes in the same commit or
> can we ship the size check removal first and then change the functionality?
> The latter would be my preference.
>
>
>
> ================
> Comment at: llvm/lib/Support/Windows/Path.inc:726
> -  if (Size > std::numeric_limits<SIZE_T>::max())
> -    return make_error_code(errc::invalid_argument);
> -
> ----------------
> rnk wrote:
> > amccarth wrote:
> > > I assume someone was originally worried that `SIZE_T` would be 32 bits
> wide in a 32-bit build but somehow believed `std::size_t` (which is the
> type of Size) could still somehow be larger than that.  Ain't gonna happen.
> > I wonder if we should turn these examples of dynamic range checks into
> static_asserts. We saw a lot of these tautological comparison warnings in
> Chrome, where it was much less obvious that the types are the same size.
> Would this be an appropriate replacement for the check?
> >   static_assert(std::numeric_limits<decltype(Size)>::max() <=
> >                 std::numeric_limits<SIZE_T>::max());
> My original plan was to come up with a way to elide the check if we can
> statically verify that it will fit, but then zturner pointed out that this
> is always true and we can just delete the check altogether.
>
> Note that the code checks if Size fits in a SIZE_T. Size was a uint64_t
> until recently and is now a size_t, which is at most a 64-bit unsigned.
> Offset is already an uint64_t. So we need a 64-bit value even if Size were
> 32-bit on some system. And we do have 64 bits, because we're passing two
> 32-bit values to CreateFileMappingW and MapViewOfFile. So a 64-bit value
> will always fit.
>
>
> https://reviews.llvm.org/D39263
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171025/fd88605b/attachment.html>