[Coin-discuss] COIN/Osi bug/portability mods, OsiDylp

[Lou Hafer]

Folks,

	The OSI layer for dylp is essentially done, modulo some polishing of
makefiles. Along the way, I've turned up a bug or two and a number of portability issues. A list, with explanations, follows. If anyone sees problems
with these (particularly the portability mods), speak up, please.

							Lou


==================================

* Common/include/CoinHelperFunctions.hpp:CoinFindDirSeparator

  Read from uninitialized error.

  The first few lines of code read:

  size_t size = 1000;
  char* buf;
  while (buf != 0) {
    buf = new char[size];

  Unfortunately, if buf just happens to come up 0, we're screwed. (This happens
  in one of the Gnu environments here.) A quick fix is to initialise buf to 1,
  as

    char* buf = reinterpret_cast<char *>(1) ;

  A better fix would be to rewrite the loop. Here's one version, you might want
  to try something different.

  inline char CoinFindDirSeparator()
  {
    size_t size = 1000;
    char* buf = new char[size] ;
    while (buf != 0)
    { if (getcwd(buf, size)) break ;
      delete[] buf ;
      size = 2*size ;
      buf = new char[size] ; }

  // if first char is '/' then it's unix and the dirsep is '/'. otherwise we 
  // assume it's dos and the dirsep is '\'

    char dirsep = (buf[0] == '/') ? '/' : '\\' ;
    delete[] buf ;
    return dirsep ;
  }

==================================

* Common/include/CoinSort.hpp:CoinSort_3

  Sun Workshop CC compiler template problem + Gnu STL bug.

  The function definition

    template <class S, class T, class U, class CoinCompare3> void
    CoinSort_3(S* sfirst, S* slast, T* tfirst, U* ufirst,
	       const CoinCompare3& tc)

  contains the following line of code:

    const int len = std::distance(sfirst, slast);
  
  Unfortunately, the Workshop CC compiler doesn't support the required
  template. It's #ifdef'd out with _RWSTD_NO_CLASS_PARTIAL_SPEC.  Looking in
  stdcomp.h, there's a line of comment that this should be defined `if your
  compiler doesn't support partial specialisation of template classes with
  default parameters.'

  The template in question is

  template <class ForwardIterator>
  inline _TYPENAME iterator_traits<ForwardIterator>::difference_type
  distance (ForwardIterator first, ForwardIterator last)
  {
    _TYPENAME iterator_traits<ForwardIterator>::difference_type n = 0;
    __distance(first, last, n,
               iterator_traits<ForwardIterator>::iterator_category());
    return n;
  }

  There's an alternate template which returns the distance through a
  reference parameter. If the code is changed to

    int len = 0 ;
    std::distance(sfirst,slast,len) ;

  CC is happy.

  The initialisation len = 0 is necessary. When I first tried to build the
  COIN unitTest in the Gnu environment, it blew up in
  OsiShallowPackedVectorTest, of all places. Once I figured out how to use gdb
  well enough to probe, I tracked the problem into CoinSort3, and finally
  into std::distance. The two versions in the Gnu library give different
  results, and it's obvious from the code. Here's the one that returns the
  distance as a return value:

  template <class _RandomAccessIterator>
  inline typename iterator_traits<_RandomAccessIterator>::difference_type
  __distance(_RandomAccessIterator __first, _RandomAccessIterator __last,
	     random_access_iterator_tag) {
    return __last - __first;
  }

  Now here's the one that returns it through a reference parameter:

  template <class _RandomAccessIterator, class _Distance>
  inline void __distance(_RandomAccessIterator __first,
			 _RandomAccessIterator __last,
			 _Distance& __n, random_access_iterator_tag)
  {
    __n += __last - __first;
  }

  Note that `+=' in the second one! If the value coming in isn't zero, you'll
  get a very interesting distance. The fix is to initialise len to 0 in
  CoinSort_3.

  While we're in the vicinity in CoinSort, the call

  CoinSort_3(sfirts, slast, tfirst, ufirst, CoinFirstLess_3<S,T,U>());
	     ^^^^^^
  is almost certainly wrong. This is around line 380, in the definition of

    template <class Iter_S, class Iter_T, class Iter_U> void
    CoinSort_3(Iter_S sfirst, Iter_S slast, Iter_T tfirst, Iter_U, ufirst)

  for the case where COIN_SORT_ARBITRARY_CONTAINERS is defined.

==========================================

* Osi/include/WarmStartBasis.hpp:setSize, setStructStatus

  Coding bugs.

  Augmented the function setSize so that it actually sets the size fields,
  in addition to allocating the status vectors:

  void setSize(int ns, int na) {
    delete[] structuralStatus_;
    delete[] artificialStatus_;
    numStructural_ = ns ;
    numArtificial_ = na ;
    structuralStatus_ = new char[(ns + 3) / 4];
    artificialStatus_ = new char[(na + 3) / 4];
    CoinFillN(structuralStatus_, (ns + 3) / 4, (char)0);
    CoinFillN(artificialStatus_, (na + 3) / 4, (char)0);
  }

  Without this modification (or some new functions to set numStructural and
  numArtificial), the only way to I could see to set them is with the
  constructor that takes sizes and status vectors as parameters. To build a
  correct warm start object, you need to create an empty one, setSize it to
  the correct size, fill in the status vectors, then construct a second
  object using the constructor that takes sizes and vectors, then destroy the
  first. Seems overly clumsy. (Alternatively, create the vectors
  independently, but that seems to violate all kinds of data-hiding
  principles.)

  Corrected the mask calculation in the function setStructStatus. It needed the
  additional `<<1' to get into the correct position.

  void setStructStatus(int i, Status st) {
    char& st_byte = structuralStatus_[i>>2];
    st_byte &= ~(3 << ((i&3)<<1));
    st_byte |= (st << ((i&3)<<1));
  }

==========================================

* Osi/include/OsiPackedVectorBase.hpp:

  Sun Workshop CC syntax problem.

  As I read the C++ standard, the `;' (circa line 145) between two
  definitions of isEquivalent should be optional. Gcc doesn't seem to care,
  but according to CC, it's a syntax error.

      return true;
   };  <--- this semicolon 
   bool isEquivalent(const OsiPackedVectorBase& rhs) const
   {

  Removing it makes CC much happier.

==========================================

* Osi/include/OsiFloatEqual.hpp:class OsiRelFltEq

  Equality and IEEE infinity.

  The original code to test equality with scaled tolerance is written as

    double tol = fabs(f1) > fabs(f2) ? fabs(f1) : fabs(f2);
    tol++;
    return fabs(f1-f2) <= epsilon_*tol; 

  Dylp follows the IEEE floating point standard and uses IEEE infinity. By
  definition, inf - inf = NaN (not a number). If you try to test inf == inf
  using the original code, the test fails.

  Arguably, this is mathematically correct: it's nonsensical to say that inf
  == inf. But, under the common trick of approximating infinity with the
  largest finite value, inf == inf will come back true and this is what the
  OSI test suite expects. Here's a possible fix. Check first for bit-for-bit
  equality. inf == inf will be caught here, as will all the other `special'
  patterns and any honest numbers which are exactly equal. Once we've done
  this, we still need to check that both numbers are finite, which requires
  the declaration

  extern "C" int finite (double) ;

  at file level (or some equivalent). The function actually comes from
  libmath, but it's declared in ieeefp.h under Sun, math.h under Gnu.

  Then the new code in RelFltEq is

    /// Compare function
    inline bool operator()(double f1, double f2) const
    { 
  /*
    Patch to handle IEEE infinity. To conform with COIN standards, we're
    declaring infty = infty. Mathematicians are rolling in their graves.
    This test for bit-for-bit equality will succeed for any numbers that are
    precisely equal, including the various `special' bit patterns.
  */
      if (f1 == f2) return true;
      if (!finite(f1) || !finite(f2)) return false;

      double tol = fabs(f1) > fabs(f2) ? fabs(f1) : fabs(f2);
      tol++;
      return fabs(f1-f2) <= epsilon_*tol; 
    };

==========================================