On Sat, Aug 18, 2018, 10:03 Akim Demaille <a...@lrde.epita.fr> wrote:
> Hi Rici! > > > Le 18 août 2018 à 16:45, Rici Lake <ricil...@gmail.com> 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).
