On Fr, 17 Nov 2023, Michael Henry wrote:
> Hi, Ernie, > > > I just tried the following as a single compare at entry > > (derived from: x * 10 + digit <= max) > > if (x > ((INT_MAX - digit) / 10)) return FAIL; > > > AFAICT, it replicates your results without a separate check > > for addition. > > Yes, I think `x > ((INT_MAX - digit) / 10)` is an accurate test > for predicting overflow. A separate check might be a little > easier to reason about, though in my experience people find it > complicated no matter how the check is implemented due to the > truncating nature of division; but I shouldn't have said a > second check was necessary, but rather that checking only `x` > (without consideration of `digit`) is insufficient to make an > accurate check. Extracting this logic into a function provides > a single location for explaining why the above test is correct, > so I see no reason not to use this more compact implementation. > > As a user, I'd prefer in general that Vim perform range > calculations accurately rather than approximately. The most > important thing is to avoid generating signed overflow, as this > leads to undefined behavior in C. Approximating the above check > using `9` instead of `digit` is conservative; it avoids all > overflow cases while disallowing some valid (but extremely > large) values. Given that these values are generally being > typed in by humans and that most uses of these values have > additional range constraints far smaller than `INT_MAX`, I can > understand the temptation to throw away a few huge values. If > this were performance-critical code, it would make sense to > consider an approximate solution. But this code is generally > running character-by-character as the user types in keystrokes, > and after converting the characters to integer values, the > subsequent operations that use these integers will in general > take much longer to execute than the integer conversion code. I > think it's unlikely the difference could be measured in a Vim > benchmark. Since a helper function like this is general-purpose > code, I'd prefer the exact implementation unless profiling > demonstrates a performance bottleneck. Thanks both. I have created the following PR to address this and another issue reported by Coverity: https://github.com/vim/vim/pull/13539 I think it should work as expected now, but please verify. Happy to give you @Michael credits for the second commit. Thanks, Chris -- The Law of the Letter: The best way to inspire fresh thoughts is to seal the envelope. -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/vim_dev/ZVdTlhfrmzzCj4od%40256bit.org.
