Re: position.hh compile error C4146 (VisualStudio 2017)

[Rici Lake]

On Sat, Aug 18, 2018, 10:03 Akim Demaille <address@hidden> wrote:

> Hi Rici!
>
> > Le 18 août 2018 à 16:45, Rici Lake <address@hidden> a écrit :
> >
> > Hi, Akim
> >
> > I'd try
> >
> >     return 0 < rhs || 0 - (static_cast<unsigned int>(rhs)) < lhs
>
> Good idea!
>
> > Or the redundant but harmless
> >
> >     return 0 < rhs || static_cast<unsigned int>(-(static_cast<unsigned
> int>(rhs))) < lhs
> >
> > C4146 is a warning, not an error. It's telling you that if u is
> unsigned, -u is still unsigned. The intent is help people avoid doing
> things like:
> >
> >     if ( i > -2147483648) ...
> >
> > which is broken (with 32-bit integers, assuming i is an int) because the
> constant is an unsigned int and therefore the comparison will be unsigned,
> which will have unexpected results. Or, at least, MS assumes the results
> will be unexpected.
> >
> > As I understand it, your code is precisely trying to avoid the undefined
> behaviour possible when using -rhs, if rhs happens to be equal to INT_MIN.
> I don’t see any problem with it, but you have to convince Visual Studio
> that you know what you're doing.
>
> I don’t remember why I wrote it this way, but today I don’t care much
> about INT_MIN here.  I think the code has become too complex for what it
> meant to do.  Also, maybe I should have sticked to int instead of trying to
> unsigned, but it’s too late to change that :)
>
>
> Yes, keeping everything int would have been easier. Or even templating the
> class on a (signed) integer type, so that applications expecting enormous
> files could deal with them.
>


> By the way, contrary to the claim in the comment in add_, that function
> only works if min is 0 or 1. If min were 2, the return value could be 1. If
> the intent is that b4_location_initial_column and b4_location_initial_line
> only have 0 or 1 as possible values, it might be clearer to make them
> booleans with names like b4_location_column_is_one_based and
> b4_location_line_is_one_based. :-)
>
> Yes, absolutely.  But I guess it’s too late too.
>
> What do you think of my proposal restoring std::max?


It's certainly clearer, although of course there are issues with integer
overflow on huge inputs (but we don't care about those, right? -:).

Since min is a parameter to add_, you can just make it int instead of
unsigned. No point in static_cast<int>(min).