[cfe-dev] [libc++] possible bug?

[Howard Hinnant]

On Apr 23, 2012, at 1:39 PM, Ovanes Markaryan wrote:

> Sorry, don't know why this email went directly to Matthieu instead of the list...
> 
> ---------- Forwarded message ----------
> From: Ovanes Markaryan <om_clang at keywallet.com>
> Date: Mon, Apr 23, 2012 at 10:30 PM
> Subject: Re: [cfe-dev] [libc++] possible bug?
> To: Matthieu Monrocq <matthieu.monrocq at gmail.com>
> 
> 
> Hello Matthieu,
> 
> please see my answers below.
> 
> On Mon, Apr 23, 2012 at 6:53 PM, Matthieu Monrocq <matthieu.monrocq at gmail.com> wrote:
> 
> 
> Le 22 avril 2012 23:58, Ovanes Markaryan <om_clang at keywallet.com> a écrit :
> 
> 
> On Sun, Apr 22, 2012 at 11:32 PM, Marc Glisse <marc.glisse at inria.fr> wrote:
> On Sun, 22 Apr 2012, Ovanes Markaryan wrote:
> 
> Hello *,
> just tried to compile SYMPHONY-5.4.4 with clang 3.0 + libc++ (tag 31). While
> compiling it I receive this error:
> 
> include/c++/v1/algorithm:643:97: error: invalid operands to binary
> expression
>       ('const reducedCost' and 'const reducedCost')
>     _LIBCPP_INLINE_VISIBILITY bool operator()(const _T1& __x, const _T1&
> __y) const {return __x < __y;}
> 
> [...]
> 
> 
>     bool operator<(const reducedCost & other)
> 
> Missing const here. It is often preferable to avoid making operators member functions when you can avoid it.
> 
>     {
>         return (value>other.value);
>     }
> 
> -- 
> Marc Glisse
> 
> Hello Marc,
> 
> you are definitely right. I overlooked it, since as already wrote this is not my code (just copy pasted it from the lib)... But in my tests I commented out this operator< and provided the specialization of  std::less, to just test it and it failed as well. I did not test with both enable (operator< and less-specialization). Actually this code is somehow really bad, because less is actually not less but greater ;) This code compiles fine with gcc and clang + gcc's version of stdlib. As far as I remember the standard requires that std::less is specialized to allow the less comparison or the operator< is implicitly used through the generalized less. In my tests less specialization was not considered by make_heap. This is my point.
> 
> 
> Two questions:
> 
> 1. Does it work if you actually specify that `operator<` is `const` ?
>  yes.
> 
> 2. Did you were careful about putting the specialization of `less` (for this class) before its first use ? (it is not clear from your example)
> Yes. Please the attached file. 
> 
> If you could, for example, provide a simple attachment that reproduces the issue it would be slightly simpler to investigate.
> 
> Here we go:
> 
> #include <algorithm>
> #include <iterator>
> #include <functional>
> 
> struct reducedCost
> {
>     /** To avoid computing two times the same row direction will have strange
>      values, direction is -1 or 1 if for only one of the two direction
>      rc is <0 and is -2 or 2 if the two direction have one <0 rc with the sign
>      indicating which one of the two directions is the best.<br>
>      Note that by theory only one reduced cost (for u_i, or v_i)
>      maybe negative for each direction.
>      */
>     int direction;
>     /** gammSign is the sign of gamma (corresponding to taking rc for u_i or v_i)
>      for the best of the two rc for this row.*/
>     int gammaSign;
>     /** gammaSign2 is the sign of gamma for the worst of the two rc for this row.*/
>     int gammaSign2;
>     /** if both reduced costs are <0 value is the smallest of the two.*/
>     double value;
>     /** greatest of the two reduced costs */
>     double value2;
>     /** index of the row.*/
>     int row;
> #ifdef TEST_WITH_OPLESS_CONST
>     bool operator<(const reducedCost & other)const
>     {
>         return (value>other.value);
>     }
> #endif
> };
> 
> #ifdef TEST_WITH_LESS_SPEC
> namespace std
> {
>     template<>
>     struct less<reducedCost>
>         : ::std::binary_function<reducedCost, reducedCost, bool>
>     {
>         bool operator()(const reducedCost & lhs, const reducedCost & rhs)const
>         {
>             return lhs.value>rhs.value;
>         }
>     };
> }
> #endif
> 
> int main()
> {
>   reducedCost r[100]={};
> 
>   std::make_heap(&r[0], &r[0]+100);
>   return 0;
> };
> 
>  
> I compiled it with clang 3.0 and libc++ taged with release_31. Here the both command lines:
> This will fail
> $ ~/toolsets/clang-3.0/bin/clang++ -DTEST_WITH_LESS_SPEC=1 -stdlib=libc++ -I/home/ovanes/toolsets/clang-3.0/include/c++/v1  -L/home/ovanes/toolsets/clang-3.0/lib make_heap_test.cpp
> This works:
> $ ~/toolsets/clang-3.0/bin/clang++ -DTEST_WITH_OPLESS_CONST=1 -stdlib=libc++ -I/home/ovanes/toolsets/clang-3.0/include/c++/v1  -L/home/ovanes/toolsets/clang-3.0/lib make_heap_test.cpp

The make_heap function overload that does not take a comparator is specified to use operator<, not less<T>, in 25.4 [alg.sorting]/p1.

Howard