[CoinUtils] Potential bug in CoinIndexedVector::reserve()

[Ted Ralphs]

Hi Robin,

Thanks for this. You said you were directed to report bugs here, but the
preferred procedure would now be to open a Github issue and I thought I had
changed most references to that by now (of course, documentation in
releases cannot be changed retroactively). If you would point out where the
reference to this list was, I can hopefully clean that up. In fact, this
list will be closing down soon in favor of Github Discussions. There is a
message from me to that effect waiting for moderator approval.

In any case, if you would not mind, can you open an issue on this?
https://github.com/coin-or/CoinUtils/issues/new

Cheers,

Ted

On Thu, Jan 7, 2021 at 9:42 AM Robin Whittle <rw at firstpr.com.au> wrote:

> Though there hasn't been a message on this list for 2 years . . . we are
> instructed to report bugs to the relevant list.
>
>
> In CoinUtils 2.11.2 and 2.11.3 - and I guess earlier versions, but I
> haven't checked - the code in CoinIndexedVector.cpp for
> CoinIndexedVector::reserve() differs from that from version 2.10.11.
> See the attached image to see how the two differ.
>
> I am using 2.10.11 and have no plans to use the later code, but I did
> notice one or two things which look wrong in these later versions.
>
> In 2.10.11, reserve() takes no action if the requested capacity == the
> current capacity.
>
>   if      (n < capacity_) {...}
>   else if (n > capacity_)
>
> This looks correct to me.  In the new versions, it takes one kind of
> action (line 541 in 2.11.3, with some added parentheses for clarity):
>
>    if ((n + nPlus) < capacity_)
>
> and the other if (line 556):
>
>    else if (n > capacity_)
>
> I suspect that line 556 should be:
>
>    else if ((n + nPlus) > capacity_)
>
> which would replicate the prior no action arrangement if n == capacity_.
>
> Functionally, the code may still work OK, but it looks wrong to me and
> there are no comments to explain why it is so.
>
> nPlus seems to be set according to the size of integers and I guess it
> represents the extra data items in the object beyond the two arrays:
>
>    int nElements_;
>    int capacity_;
>    int offset_;
>    bool packedMode_;
>
> Also, given line 541, it seems odd that line 585 remains:
>
>   capacity_ = n;
>
> The intended functionality of the various parts of this function could
> have been clearly described with suitable comments.  Then I wouldn't
> have to guess about what it is supposed to do.  In the absence of such
> comments I can't know for sure what it supposed to do, but I wanted to
> point out what seems to me like potential problems.
>
> Robin
> _______________________________________________
> CoinUtils mailing list
> CoinUtils at list.coin-or.org
> https://list.coin-or.org/mailman/listinfo/coinutils
>


-- 
Dr. Ted Ralphs
Professor, Lehigh University
(610) 628-1280
ted 'at' lehigh 'dot' edu
coral.ie.lehigh.edu/~ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.coin-or.org/pipermail/coinutils/attachments/20210107/afae54e3/attachment-0001.html>