COIN-OR::LEMON - Graph Library

Opened 10 years ago

Closed 6 years ago

#294 closed defect (fixed)

ignore_unused_variable_warning

Reported by: chrisatlemon Owned by: Balazs Dezso
Priority: major Milestone: LEMON 1.3 release
Component: core Version: hg main
Keywords: Cc:
Revision id:

Description

I noticed that both in #include <boost/graph/graph_concepts.hpp> and in #include <lemon/graph_concepts.hpp>

the helper template function "ignore_unused_variable_warning" is defined! Trying to write an BGL adaption of a ListGraph?, for me gcc complains e.g. about

"call of overloaded 'ignore_unused_variable_warning(lemon::ListGraphBase::Node&)' is ambiguous"

I attached a minimal code example which reproduces the error

Attachments (3)

testgraph.cpp (1.7 KB) - added by chrisatlemon 10 years ago.
testgraph.cc (303 bytes) - added by Balazs Dezso 10 years ago.
shorter example
9029c764a07c.patch (13.8 KB) - added by Balazs Dezso 6 years ago.

Download all attachments as: .zip

Change History (23)

Changed 10 years ago by chrisatlemon

Attachment: testgraph.cpp added

comment:1 in reply to:  description ; Changed 10 years ago by Alpar Juttner

Milestone: LEMON 1.2 release

The two instances of ignore_unused_variable_warning() are in different namespaces, thus it should not cause conflicts.

Are you sure that this is the real root of the problems. For example I can see other errors preceding the one you refer to, see below.

g++ testgraph.cpp $(pkg-config --cflags --libs /home/alpar/projects/LEMON/local/lib/pkgconfig/lemon.pc)
/usr/include/boost/graph/graph_concepts.hpp: In destructor 'boost::concepts::VertexListGraph<G>::~VertexListGraph() [with G = lemon::ListGraph]':
/usr/include/boost/graph/graph_concepts.hpp:164:   instantiated from 'static void boost::concept::requirement<Model>::failed() [with Model = boost::concepts::VertexListGraphConcept<lemon::ListGraph>]'
/usr/include/boost/concept_check.hpp:43:   instantiated from 'void boost::function_requires(Model*) [with Model = boost::concepts::VertexListGraphConcept<lemon::ListGraph>]'
testgraph.cpp:74:   instantiated from here
/usr/include/boost/graph/graph_concepts.hpp:185: error: no matching function for call to 'vertices(lemon::ListGraph&)'
/usr/include/boost/graph/graph_concepts.hpp: In member function 'void boost::concepts::VertexListGraph<G>::const_constraints(const G&) [with G = lemon::ListGraph]':
/usr/include/boost/graph/graph_concepts.hpp:187:   instantiated from 'boost::concepts::VertexListGraph<G>::~VertexListGraph() [with G = lemon::ListGraph]'
/usr/include/boost/graph/graph_concepts.hpp:164:   instantiated from 'static void boost::concept::requirement<Model>::failed() [with Model = boost::concepts::VertexListGraphConcept<lemon::ListGraph>]'
/usr/include/boost/concept_check.hpp:43:   instantiated from 'void boost::function_requires(Model*) [with Model = boost::concepts::VertexListGraphConcept<lemon::ListGraph>]'
testgraph.cpp:74:   instantiated from here
/usr/include/boost/graph/graph_concepts.hpp:199: error: no matching function for call to 'vertices(const lemon::ListGraph&)'

Changed 10 years ago by Balazs Dezso

Attachment: testgraph.cc added

shorter example

comment:2 Changed 10 years ago by Balazs Dezso

If the ignore_unused_variable_warning() function is called in the boost namespace and the parameter is in the lemon namespace, then the Koenig lookup will find both functions.

I have uploaded a shorter example, which shows only this problem.

I think, it is better to rename this function.

comment:3 in reply to:  1 Changed 10 years ago by Alpar Juttner

Replying to alpar:

For the record, I copy here Chris' answer from the lemon-devel list.

Hello,

The two instances of ignore_unused_variable_warning() are in different namespaces, thus it should not cause conflicts.

I think this can indeed cause problems because of the "argument dependent name lookup" (see e.g. "http://en.wikipedia.org/wiki/Argument_dependent_name_lookup") which makes the lemon version avaiable to the boost namespace as the given object also comes from the lemon namespace.

For the other errors I habe to apologize, they seem to be purely boost related (I'm currently investigating this)

greetings, chris

comment:4 in reply to:  2 ; Changed 10 years ago by Alpar Juttner

Replying to deba:

I think, it is better to rename this function.

I'd rather think the problem on the Boost's side. If they want to use their own ignore_unused_variable_warning() for third party classes, they should give the boost namespace explicitly, like

boost::ignore_unused_variable_warning(x);

or even like

::boost::ignore_unused_variable_warning(x);

Cris, could you confirm (or confute) this?

comment:5 in reply to:  4 ; Changed 10 years ago by chrisatlemon

Replying to alpar:

Cris, could you confirm (or confute) this?

This would be the cleanest solution to the problem but renaming the easiest, I think, as this would not require to convince the developers on boost's side.

comment:6 in reply to:  4 ; Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to deba:

I think, it is better to rename this function.

I'd rather think the problem on the Boost's side. If they want to use their own ignore_unused_variable_warning() for third party classes, they should give the boost namespace explicitly, like

boost::ignore_unused_variable_warning(x);

or even like

::boost::ignore_unused_variable_warning(x);

Cris, could you confirm (or confute) this?

In my opinion, this is a quite deep problem in C++. Let us imagine, that the boost Dijkstra algorithm is called dijkstra(). (Fortunately, it is dijkstra_shortest_path()). If similarly the dijkstra were called from the boost namespace on a LEMON graph, then we would get the same problem. Because, you cannot know which names will be reused in other libraries in different namespaces, therefore you should explicitly specify the namespace of each function, which takes template parameters. In addition, following your advice about specifying the namespace of ignore_unused_variable_warning() in boost, it should be done for each template function in boost, lemon and all template libraries.

This is a design problem in C++, the effect of Koenig lookup were not checked against templates.

comment:7 in reply to:  6 Changed 10 years ago by Alpar Juttner

Replying to deba:

In my opinion, this is a quite deep problem in C++.

I agree, that this is a superb stupidity in C++, but your example is probably wrong. The name collision causes problems when you the function from within the same namespace, and its parameter (type) comes from a third party namespace. I don't think this could be the case with dijsktra().

Anyway, if there is a change that it may happen, you must use it the explicitly given namespace. This is what boost should to do with ignore_unused_variable_warning.

comment:8 in reply to:  5 ; Changed 10 years ago by Alpar Juttner

Replying to chrisatlemon:

This would be the cleanest solution to the problem but renaming the easiest, I think, as this would not require to convince the developers on boost's side.

Well, AFAIK, Boost guy's are big C++ advocates. So, their attitude should be very positive towards the correct use of the language. At least it worth trying to get in touch with them. Any volunteer for this task?

Btw. I have some fear that we will run into the same problem with enable_if. If this is true, then it gonna be a major headache.

comment:9 in reply to:  8 ; Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to chrisatlemon:

This would be the cleanest solution to the problem but renaming the easiest, I think, as this would not require to convince the developers on boost's side.

Well, AFAIK, Boost guy's are big C++ advocates. So, their attitude should be very positive towards the correct use of the language. At least it worth trying to get in touch with them. Any volunteer for this task?

Btw. I have some fear that we will run into the same problem with enable_if. If this is true, then it gonna be a major headache.

The enable_if is not affected by this problem, because that is a class. My proposal is to change the ignore_unused_variable_warning to a static template member function, because for them the Koenig lookup is not used.

comment:10 in reply to:  9 ; Changed 10 years ago by Alpar Juttner

Replying to deba:

Replying to alpar:

Replying to chrisatlemon:

This would be the cleanest solution to the problem but renaming the easiest, I think, as this would not require to convince the developers on boost's side.

Well, AFAIK, Boost guy's are big C++ advocates. So, their attitude should be very positive towards the correct use of the language. At least it worth trying to get in touch with them. Any volunteer for this task?

Btw. I have some fear that we will run into the same problem with enable_if. If this is true, then it gonna be a major headache.

The enable_if is not affected by this problem, because that is a class. My proposal is to change the ignore_unused_variable_warning to a static template member function, because for them the Koenig lookup is not used.

You mean you would like to put a copy of this function into each class it is used? I don't like this idea very much.

Instead, I would still prefer persuading the Boost people somehow to use their ignore_unused_variable_warning properly. Has anyone made an attempt in this direction? (Sending an e-mail to an appropriate list, issuing a bug report etc.)

comment:11 in reply to:  10 ; Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to deba:

Replying to alpar:

Replying to chrisatlemon:

This would be the cleanest solution to the problem but renaming the easiest, I think, as this would not require to convince the developers on boost's side.

Well, AFAIK, Boost guy's are big C++ advocates. So, their attitude should be very positive towards the correct use of the language. At least it worth trying to get in touch with them. Any volunteer for this task?

Btw. I have some fear that we will run into the same problem with enable_if. If this is true, then it gonna be a major headache.

The enable_if is not affected by this problem, because that is a class. My proposal is to change the ignore_unused_variable_warning to a static template member function, because for them the Koenig lookup is not used.

You mean you would like to put a copy of this function into each class it is used? I don't like this idea very much.

Instead, I would still prefer persuading the Boost people somehow to use their ignore_unused_variable_warning properly. Has anyone made an attempt in this direction? (Sending an e-mail to an appropriate list, issuing a bug report etc.)

What does it mean that using it properly? In my point of view, if I define a function in a namespace (for example in lemon namespace), then I can call it without specifying its namespace from lemon.

Anyway, if the boost code is wrong, then our code should be wrong as well. So we have to fix our code. There are a few solutions.

  • We have to specify lemon namespace for our function:
      ::lemon::ignore_unused_variable_warning(t);
    

If we choose this variant then boost have to solve the problem similarly. In addition if a function is defined in lemon namespace and it is called from the same namespace and it is unconstrained, then it has to be called with specifying its namespace.

  • The second option to change the name of the function:
      lemon_ignore_unused_variable_warning(t);
    

I thought that we can forget the prefixes with the introduction of namespaces in c++, but the lemon prefix can help to avoid the name collision.

  • We can wait to the C++ committee while they drop the Koenig lookup rule for unconstrained template functions.
  • Using class based implementation:
      struct IgnoreUnusedVariableWarning {
        template <typename T>
        void ignore(const T &t) { } 
      };
      ...
      IgnoreUnuseVariableWarning::ignore(t);
    
  • The previous solution with defines:
      #define LEMON_IGNORE_UNUSED_VARIABLE_WARNING(t) IgnoreUnuseVariableWarning::ignore(t)
    

comment:12 in reply to:  11 ; Changed 10 years ago by Alpar Juttner

Replying to deba:

What does it mean that using it properly? In my point of view, if I define a function in a namespace (for example in lemon namespace), then I can call it without specifying its namespace from lemon.

It would be nice, but alas it is not true, exactly because of the Koenig lookup.

Anyway, if the boost code is wrong, then our code should be wrong as well.

I don't think so. I have checked the usage of ignore_unused_variable_warning() at random places and found no problem with the usage. Recall that it is necessary to declare the namespace only if there is a chance that it will be used with a type coming from another namespace. I could not find any example for this kind of usage (though I haven't checked all). Could you?

So we have to fix our code.

Did you indeed find any problematic code?

There are a few solutions.

  • We have to specify lemon namespace for our function:
      ::lemon::ignore_unused_variable_warning(t);
    

Yes, I prefer this.

If we choose this variant then boost have to solve the problem similarly.

Yes, they have.

In addition if a function is defined in lemon namespace and it is called from the same namespace and it is unconstrained, then it has to be called with specifying its namespace.

I can't understand this.

comment:13 in reply to:  12 Changed 10 years ago by Balazs Dezso

Replying to alpar:

Replying to deba:

What does it mean that using it properly? In my point of view, if I define a function in a namespace (for example in lemon namespace), then I can call it without specifying its namespace from lemon.

It would be nice, but alas it is not true, exactly because of the Koenig lookup.

Anyway, if the boost code is wrong, then our code should be wrong as well.

I don't think so. I have checked the usage of ignore_unused_variable_warning() at random places and found no problem with the usage. Recall that it is necessary to declare the namespace only if there is a chance that it will be used with a type coming from another namespace. I could not find any example for this kind of usage (though I haven't checked all). Could you?

You miss one point, lets see the following example, let's take that boost have some bignum class or some other ordered number type (boost::BigNum?). The dijkstra should check that the heap is conform to the heap concept. If somebody want to call the dijkstra with BigNum? value type.

  lemon::Dijkstra<SmartDigraph, SmartDigraph::ArcMap<boost::BigNum> >
    ::SetHeap<FibHeap<boost::BigNum, SmartDigraph::ArcMap<int> >
    ::Create dijkstra(digraph, length);

Because the heap concept check uses the ignore_unused_variable_warning() for the value type of the heap, and it can be defined in any namespaces (in this example in the boost namespace), therefore our ignore_unused_variable_warning() is wrong.

So we have to fix our code.

Did you indeed find any problematic code?

There are a few solutions.

  • We have to specify lemon namespace for our function:
      ::lemon::ignore_unused_variable_warning(t);
    

Yes, I prefer this.

If we choose this variant then boost have to solve the problem similarly.

Yes, they have.

In addition if a function is defined in lemon namespace and it is called from the same namespace and it is unconstrained, then it has to be called with specifying its namespace.

I can't understand this.

We do not restrict which classes are lemon graphs, we define only the interface of the graph in the graph concepts. If somebody defined a graph in the boost namespace which is conform to our graph concept, and the boost defined a countNodes() function, then in our algorithms the countNodes() function calls would become ambiguous. Therefore, most of the LEMON functions can be affected with this problem (at least which are unconstrained and called from the library itself).

comment:14 Changed 10 years ago by Peter Kovacs

Owner: changed from Alpar Juttner to Balazs Dezso

I like the class based solution. However, we could write to Boost developers about this problem.

comment:15 in reply to:  14 Changed 10 years ago by Alpar Juttner

Replying to kpeter:

I like the class based solution. However, we could write to Boost developers about this problem.

What is the advantage of the class based solution over simply adding the full explicit namespace?

comment:16 Changed 9 years ago by Alpar Juttner

What shall we do with this ticket?

comment:17 Changed 9 years ago by Alpar Juttner

Milestone: LEMON 1.2 releaseLEMON 1.3 release

comment:18 Changed 6 years ago by Alpar Juttner

What shall we do with this ticket? Any feedback from the Boost guys?

Changed 6 years ago by Balazs Dezso

Attachment: 9029c764a07c.patch added

comment:19 Changed 6 years ago by Balazs Dezso

I have added a changeset to add explicitly namespace to the ignore_unused_variable_warning() calls.

comment:20 Changed 6 years ago by Alpar Juttner

Resolution: fixed
Status: newclosed

Thanks. I split the modification to more changesets so that it can be merged to all active branches, see [3e711ee55d31], [a337a0dd3f75] and [dd1443e4a34c].

Note: See TracTickets for help on using tickets.