COIN-OR::LEMON - Graph Library

Opened 12 years ago

Closed 12 years ago

#38 closed defect (fixed)

Revise constMap() functions

Reported by: Peter Kovacs Owned by: Balazs Dezso
Priority: major Milestone: LEMON 1.0 release
Component: core Version:
Keywords: Cc:
Revision id:

Description

One version of the overloaded constMap() function can't be called. It could be used to create a ConstMap instance with inlined constant value.

  • ConstMap<int,Const<int,10> > cm; works, but
  • constMap<int,Const<int,10> >(); does not work. It causes a compiler error: no matching function for call to ‘constMap()’.

Attachments (1)

constmap.patch (2.5 KB) - added by Peter Kovacs 12 years ago.
Patch for constMap() functions and FunctorToMap?

Download all attachments as: .zip

Change History (10)

comment:1 Changed 12 years ago by Alpar Juttner

Owner: changed from Alpar Juttner to Balazs Dezso

comment:2 Changed 12 years ago by Balazs Dezso

The const map with static value should be used in the following syntax: constMap<int, int, 10>();

In my point of view, it is a nicer syntax, but it could get more emphasis in the documentation.

Changed 12 years ago by Peter Kovacs

Attachment: constmap.patch added

Patch for constMap() functions and FunctorToMap?

comment:3 Changed 12 years ago by Peter Kovacs

Replying to deba:

The const map with static value should be used in the following syntax: constMap<int, int, 10>();

You helped me a lot with this example.

In my point of view, it is a nicer syntax,

Nicer or not, it works. Now I understand it, but it is not clear about the documentation, as you said.

I made a patch file to solve these problems. I added a new version of the constMap() function without parameter, so there are three versions:

  template<typename K, typename V>
  inline ConstMap<K, V> constMap(const V &v);
  
  template<typename K, typename V>
  inline ConstMap<K, V> constMap();

  template<typename K, typename V, V v>
  inline ConstMap<K, Const<V, v> > constMap();

Now both constMap<int, int, 10>() and constMap<int, Const<int, 10> >() works poperly. Although the former one calls the third version and the latter one calls the second version of the function, they are equivalent.

Apart form that I made another small fix in FunctorToMap. (The test file failed when it was compiled without optimization.)

What is your opinion? I think accepting this patch we needn't modify the documentation, since both syntax versions works. Is it okay?

comment:4 Changed 12 years ago by Peter Kovacs

Priority: minormajor

comment:5 Changed 12 years ago by Alpar Juttner

What is the reason for this patch? Should it be included in the main?

comment:6 in reply to:  3 ; Changed 12 years ago by Balazs Dezso

I made a patch file to solve these problems. I added a new version of the constMap() function without parameter, so there are three versions:

  template<typename K, typename V>
  inline ConstMap<K, V> constMap(const V &v);
  
  template<typename K, typename V>
  inline ConstMap<K, V> constMap();

  template<typename K, typename V, V v>
  inline ConstMap<K, Const<V, v> > constMap();

Now both constMap<int, int, 10>() and constMap<int, Const<int, 10> >() works poperly. Although the former one calls the third version and the latter one calls the second version of the function, they are equivalent.

I think both formats could be accepted for static const map values, therefore I support the third variant.

Apart form that I made another small fix in FunctorToMap. (The test file failed when it was compiled without optimization.)

What is your opinion? I think accepting this patch we needn't modify the documentation, since both syntax versions works. Is it okay?

The standard library stores the functors as value and not as reference. So Peter's solution is more convenient and conform to the c++ standard library, so I support to reverse this change.

comment:7 in reply to:  6 Changed 12 years ago by Peter Kovacs

Maybe it wasn't clear, but there are two independent modifications in the patch.

  • A new variant of constMap() without parameter (that was the second variant on my list) to support constMap<A,B>() calls. E.g. it makes possible to call constMap<A,Const<int,10> >(), which was not supported before.
  • A bug fix in FunctorToMap: store the functor as value instead of reference. That bug was made by me during the general clean-up (the test file passed with optimization, so I didn't notice the problem).

So Peter's solution is more convenient and conform to the c++ standard library, so I support to reverse this change.

The patch just restores the solution used in the svn repository.

I think this bug fix is not a question. The only question is about the usage of constMap(). Should we also support constMap<A,B>() or just constMap<A,B>(b) and constMap<A,B,b>()? (The first one makes possible to call constMap<A,Const<B,b> >().)

comment:8 Changed 12 years ago by Balazs Dezso

I think the patch solves the problem, so I think it should be merged into the main repository, and this thread could be closed.

comment:9 in reply to:  5 Changed 12 years ago by Alpar Juttner

Resolution: fixed
Status: newclosed

This patch in now in the main branch, see [8899d1891a3c]

Note: See TracTickets for help on using tickets.