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

[Ted Ralphs]

FYI, here is the commit that introduced the commit:

https://github.com/coin-or/CoinUtils/commit/2851f9c872c413d2f1ecbc77429d47dfa17fe1ad

Yes, looks like it was just buggy.

Ted

On Thu, Jan 7, 2021 at 9:58 AM Ted Ralphs <tkralphs at lehigh.edu> wrote:

> 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
>


-- 
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/8521dcff/attachment.html>