Re: [lmi] expand_html() msw64: 'SIZE_MAX <= index' always false

[Vadim Zeitlin]

On Sun, 24 Mar 2019 19:18:17 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-24 17:00, Vadim Zeitlin wrote:
GC> > On Sun, 24 Mar 2019 10:09:21 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> Vadim--is this the right fix for the problem below?
GC> [...]
GC> > GC> -            auto const index = std::strtoul(s.c_str() + open_pos + 
1, &stop, 10);
GC> > GC> +            auto const index = std::strtoull(s.c_str() + open_pos + 
1, &stop, 10);
GC> [...]
GC> > GC> /opt/lmi/src/lmi/pdf_command_wx.cpp:278:63: error: comparison is 
always false due to limited range of data type [-Werror=type-limits]
GC> > GC>              if(stop != s.c_str() + s.length() - 1 || SIZE_MAX <= 
index)
GC> [...]
GC> >  Sorry, I'm not sure why would we want to use long long here when the
GC> > indices are typically small integers, i.e. should fit in a short if not
GC> > char.
GC> 
GC> I figured that you had chosen SIZE_MAX because you wanted type std::size_t,
GC> much as we compare std::string::find() results to std::string::npos.

 But find() is documented to return npos, while std::strtoul() is
documented to return ULONG_MAX in case of overflow. And I [know that I]
know it. So this can't be the explanation -- but the commit 416ab02f that
you have so obligingly found for me does provide one:

 In this commit, the check preceded a static_cast of index to size_t, so
I must have just decided to make it absolutely manifest that the cast was
valid and didn't realize that this could result in the problem when
ULONG_MAX < SIZE_MAX.

GC> > The real problem seems to be a rather head-scratching use of SIZE_MAX
GC> > here, to be honest I have absolutely no recollection why did I decide to 
do
GC> > it (and out Git history doesn't contain the details of my commits in that
GC> > branch, so even if I had written a message explaining it back when I added
GC> > it, it's not available any longer...).
GC> 
GC> Really? I thought we had taken great care to preserve your commits. Thus,
GC> these two commits are distinct [found by 'git log --all -G'SIZE_MAX'']:

 Thanks a lot for finding it. I had just use git-blame and it stopped at
the first commit (a78b402a), which created the file, so I didn't realize
that the other commit was still there too.

GC> commit a78b402a87d2954de9bc8bf98ecf0ccbe75cb294
GC> Author: Gregory W. Chicares <address@hidden>
GC> Date:   2018-01-27T07:08:11+00:00
GC> 
GC>     Import remaining new files verbatim from vz-no-xslfo
GC> 
GC> commit 416ab02fb8b8ea4da036fa86d27fdf19772f6a69
GC> Author: Vadim Zeitlin <address@hidden>
GC> Date:   2017-06-30T01:59:13+00:00
GC> 
GC>     Add support for vector variables to PDF generating code
GC>     
GC>     Recognize anything of the form "name[index]" as a vector.
GC> 
GC> Doesn't 'git show 416ab02fb8' display one of your original,
GC> separate commits?

 Yes, it does, and it explains why I did it like this, so now my curiosity
is satisfied and we just have to actually fix the problem...

GC> >  Looking at the code anew it seems that we should really be using 
ULONG_MAX
GC> > instead, which wouldn't generate this warning (I've tested and confirmed
GC> > that this is indeed the case). I.e. I'd rather propose this patch instead:
GC> 
GC> Committed, thanks.

... which you've now done as well.

 Thank you!
VZ