COIN-OR::LEMON - Graph Library

Opened 10 years ago

Closed 10 years ago

#283 closed defect (fixed)

AIX/xlC compilation failure in euler_test.h

Reported by: Alpar Juttner Owned by: Peter Kovacs
Priority: blocker Milestone: LEMON 1.1 release
Component: core Version: hg main
Keywords: Cc: held@…
Revision id: e01957e96c67

Description

Please find the compiler output below

source='test/euler_test.cc' object='test/euler_test.o' libtool=no \
DEPDIR=.deps depmode=aix /bin/sh ./build-aux/depcomp \
/fs/data/edatools/prod/tools/13.1/bin/xlC -DHAVE_CONFIG_H   -I. -I.   -g -c -o test/euler_test.o test/euler_test.cc
"test/euler_test.cc", line 71.23: 1540-0229 (S) The best viable function "lemon::VectorMap<lemon::DigraphExtender<lemon::ListDigraphBase>,lemon::ListDigraphBase::Arc,int>::operator[](const Key &)" uses an ambiguous conversion sequence.
"test/euler_test.cc", line 56.6: 1540-0700 (I) The previous message was produced while processing "checkEulerIt<lemon::Undirector<lemon::ListDigraph> >(const Undirector<lemon::ListDigraph> &, const Node &)".
"test/euler_test.cc", line 203.5: 1540-0700 (I) The previous message was produced while processing "main()".
gmake[2]: *** [test/euler_test.o] Error 1

Attachments (3)

euler-aix-9a9629600c20.patch (1.3 KB) - added by Peter Kovacs 10 years ago.
adaptors_fix.bundle (28.6 KB) - added by Balazs Dezso 10 years ago.
This change solves the problem in euler
283-bf7928412136.patch (1.1 KB) - added by Peter Kovacs 10 years ago.

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by Peter Kovacs

comment:1 Changed 10 years ago by Peter Kovacs

[9a9629600c20] hopefully solves this compilation problem (and other small bugs).

comment:2 Changed 10 years ago by Peter Kovacs

Owner: changed from Alpar Juttner to Peter Kovacs
Status: newassigned

Changed 10 years ago by Balazs Dezso

Attachment: adaptors_fix.bundle added

This change solves the problem in euler

comment:3 Changed 10 years ago by Balazs Dezso

There are several solutions for the problem, however neither of them are totally clean.

  • The Edge conversion in EulerIt? can be generated only if the Arc is not convertible to Edge. It would be a nice template hacking.
  • The Edge conversion can be deleted from EulerIt?, and if it should be converted to Edge then an explicit Arc conversion should be used.
  • The inheritance between Undirector::Edge and Undirector::Arc can be changed to conversion.
  • The EulerIt? can be inherited from Arc.
  • Explicit conversion call to avoid ambiguity.

The last solution did not work on Aix, the first and fourth were disapproved. The second and the third worked also on Aix. The second is quite strange (however it is comprehensible), because we would like to convert to Edge, but we have to convert to Arc. Last, the third were implemented in adaptors_fix.bundle.

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

Replying to deba:

Last, the third were implemented in adaptors_fix.bundle.

I am a bit concerned of this solution. The problem is that I feel the problem is in EulerIt but instead we fixed Undirector. We did it only because we tested EulerIt with it. However similar problem could also appear with other graph adaptors, couldn't it? Moreover, the original implementation of Undirector seems perfectly fit the undirected graph concept, therefore it is strange to say that it is buggy.

Do I understand correctly that Peter's patch does not fix the problem? Why?

comment:5 in reply to:  4 Changed 10 years ago by Peter Kovacs

Replying to alpar:

I am a bit concerned of this solution. The problem is that I feel the problem is in EulerIt but instead we fixed Undirector. We did it only because we tested EulerIt with it. However similar problem could also appear with other graph adaptors, couldn't it?

I checked this question. Undirector is the only graph type having Arc inherited from Edge except for the graph concept class (Graph) and UndirDigraphExtender in bits/base_extender.h. However this file is not used by any other source files, thus I think it should be removed from the repository. (As far as I remember this question was considered before, but I don't know what the conlcusion was.)

Therefore I think Balazs's solution is correct, but we should also modify the Graph class in order to make it consistent with the graph realizations.

Moreover, the original implementation of Undirector seems perfectly fit the undirected graph concept, therefore it is strange to say that it is buggy.

Yes it does, but other graph structures, such as ListGraph, SmartGraph, FullGraph have an Arc type that is not inherited form Edge, so we could say that Undirector and Graph is "buggy", or at least incosistent with the oter classes.

Do I understand correctly that Peter's patch does not fix the problem? Why?

As far as I know, Balazs tested this solution along with a similar solution using static_cast, but neither of them passed on AIX. It is not enough to force the Edge() conversion.

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

Component: corebuild system

Replying to alpar:

Replying to deba:

Last, the third were implemented in adaptors_fix.bundle.

I am a bit concerned of this solution. The problem is that I feel the problem is in EulerIt but instead we fixed Undirector. We did it only because we tested EulerIt with it. However similar problem could also appear with other graph adaptors, couldn't it? Moreover, the original implementation of Undirector seems perfectly fit the undirected graph concept, therefore it is strange to say that it is buggy.

Yes, it disturbed me also. I think the problem is neither in the Undirector nor in the EulerIt?, I think it is in the C++ 'one implicit conversion rule', but unfortunately I do not have write access for the C++ standard :). Nevertheless, I must validate, I do not think that the Undirector were buggy.

Do I understand correctly that Peter's patch does not fix the problem? Why?

It did not solve the problem because the explicit casting just forced the same cast as it would be made automatically.

Other good option is the second, maybe that could be applied instead of third.

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

Component: build systemcore

Replying to deba:

Other good option is the second, maybe that could be applied instead of third.

Or both. Just to be on the safe side. Opinion?

comment:8 in reply to:  6 ; Changed 10 years ago by Peter Kovacs

Replying to deba:

Other good option is the second, maybe that could be applied instead of third.

It would be safe, but really not user-friendly. If the user would like to convert to Edge, then an explicit Arc() conversion would be necessary, while if Arc is needed, no explicit conversion would be necessary. I do not like this solution from the user's point of view.

What about Balazs's original patch along with modifying the Graph concept?

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

Replying to kpeter:

Replying to deba:

Other good option is the second, maybe that could be applied instead of third.

It would be safe, but really not user-friendly. If the user would like to convert to Edge, then an explicit Arc() conversion would be necessary, while if Arc is needed, no explicit conversion would be necessary. I do not like this solution from the user's point of view.

I don't really understand the situation. Does it mean that the Arc->Edge conversion does not work at all with xlC? For example, does

Edge e=EulerIt;

compile or not (assuming we remove operator Edge() from EulerIt?)?

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

Edge e=EulerIt;

I mean

Edge e=euler_it;

comment:11 in reply to:  9 Changed 10 years ago by Peter Kovacs

Replying to alpar:

I don't really understand the situation. Does it mean that the Arc->Edge conversion does not work at all with xlC? For example, does Edge e=EulerIt; compile or not (assuming we remove operator Edge() from EulerIt?)?

As far as I understand it wouldn't compile for almost all graph classes using any compliers if we remove operator Edge() from EulerIt, since in this case two implicit conversions have to be made. One for EdgeIt --> Arc and another one for Arc --> Edge. The later one is a conversion for almost all graph types, except for Undirector and Graph, in which Arc is inherited from Edge.

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

The contents of adaptors_fix.bundle has been reapplied to the tip of the main branch (with modified chgset log), see [cb38ccedd2c1]. It will be merges to the release branch along with the other chgset.

Changed 10 years ago by Peter Kovacs

Attachment: 283-bf7928412136.patch added

comment:13 Changed 10 years ago by Peter Kovacs

[bf7928412136] also changes Graph::Edge -> Graph::Arc inheritance to conversion, to be consistent with the realizations.

comment:14 Changed 10 years ago by Alpar Juttner

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.