COIN-OR::LEMON - Graph Library

Opened 11 years ago

Closed 10 years ago

#17 closed task (fixed)

Revise debug modes and exceptions

Reported by: alpar Owned by: deba
Priority: major Milestone: LEMON 1.0 release
Component: core Version: hg main
Keywords: Cc: ladanyi
Revision id:

Description

We should revise the debug modes and organizations of the exceptions.
We need clear rules in which cases should we throw extensions.

Attachments (7)

7abfb55f1ecc.patch (7.2 KB) - added by deba 10 years ago.
removing fixme and log
305d03f9bcea.patch (43.0 KB) - added by deba 10 years ago.
d91884dcd572.patch (1.7 KB) - added by kpeter 10 years ago.
using DEBUG instead of ASSERT in graph extenders
8bed1c3a6080.patch (31.0 KB) - added by alpar 10 years ago.
f6899946c1ac.patch (55.1 KB) - added by deba 10 years ago.
error_d901321d6555.patch (20.5 KB) - added by kpeter 10 years ago.
7c796c1cf1b0.patch (5.6 KB) - added by deba 10 years ago.
Bug fix in exception throwing

Download all attachments as: .zip

Change History (37)

comment:1 Changed 11 years ago by kpeter

  • Version set to hg main

comment:2 Changed 10 years ago by alpar

  • Owner changed from alpar to kpeter

comment:3 follow-up: Changed 10 years ago by alpar

This ticket is more-or-less a duplicate of ticket:28, isn't it?

Are there any open issues remained?
Can we close this ticket?

comment:4 in reply to: ↑ 3 Changed 10 years ago by kpeter

Replying to alpar:

This ticket is more-or-less a duplicate of ticket:28, isn't it?
Are there any open issues remained?

The assertion modes and tools (assert.h) have been revised (see: #28), but error.h still needs revision. E.g. exceptions like RuntimeError, RangeError, IoError, UninitializedParameter etc.

comment:5 Changed 10 years ago by kpeter

error_test.cc should also be extended with test cases for the tools of error.h, since it contains tests only for the assertion system now.

comment:6 in reply to: ↑ description ; follow-up: Changed 10 years ago by alpar

Replying to alpar:

We should revise the debug modes and organizations of the exceptions.
We need clear rules in which cases should we throw extensions.

I had a short look at lemon/assert.h and I still found some garbage in that. For example

  • For me LEMON_FIXME is completely unnecessary.
  • It is written in the doc that LEMON_DEBUG works exactly the same LEMON_ASSERT does. It shouldn't be true.
  • Do we really need the LEMON_ASSERT_LOG and the LEMON_ASSERT_CUSTOM options? They makes everything very complex and (for me) of little use.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 10 years ago by kpeter

Replying to alpar:

  • For me LEMON_FIXME is completely unnecessary.

I agree. LEMON_FIXME shouldn't be used like \todo commands. We should create tickets instead.

  • It is written in the doc that LEMON_DEBUG works exactly the same LEMON_ASSERT does. It shouldn't be true.

I think it is not written in the doc. The doc says "This macro works like the LEMON_ASSERT macro", the only difference is that LEMON_DEBUG is disabled by default, since it is used for other purposes. But this difference is also written in the doc.

However I also found these macros and their doc a bit too complicated.

  • Do we really need the LEMON_ASSERT_LOG and the LEMON_ASSERT_CUSTOM options? They makes everything very complex and (for me) of little use.

Maybe LEMON_ASSERT_LOG can be useful sometimes, but I do not find it really important. LEMON_ASSERT_CUSTOM seems to be complex, but it provides more general usage.
I think LEMON_ASSERT_LOG can be removed if it doesn't seem to be really useful.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 10 years ago by alpar

Replying to kpeter:

LEMON_ASSERT_CUSTOM seems to be complex, but it provides more general usage.

But do we need this generality? Why is it useful?

comment:9 Changed 10 years ago by kpeter

  • Owner changed from kpeter to deba

comment:10 Changed 10 years ago by ladanyi

  • Cc ladanyi added

comment:11 in reply to: ↑ 8 ; follow-up: Changed 10 years ago by deba

Replying to alpar:

Replying to kpeter:

LEMON_ASSERT_CUSTOM seems to be complex, but it provides more general usage.

But do we need this generality? Why is it useful?

For example you can change the assertion handler function like the following.

{

MessageBox::showMessage("A fatal error occurred in Lemon Library", "Error");
std::ofstream os("error.log");
os << msg << std::endl;

}
exit(0);

In a graphical environment it could much better solution just than calling an abort().

comment:12 in reply to: ↑ 11 ; follow-up: Changed 10 years ago by kpeter

Replying to deba:

For example you can change the assertion handler function like the following.

What about removing LEMON_FIXME and maybe LEMON_ASSERT_LOG, too, but keeping LEMON_ASSERT_ABORT (the default behavior) and LEMON_ASSERT_CUSTOM?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 10 years ago by alpar

Replying to kpeter:

What about removing LEMON_FIXME

We should certainly revome LEMON_FIXME

and maybe LEMON_ASSERT_LOG, too, but keeping LEMON_ASSERT_ABORT (the default behavior) and LEMON_ASSERT_CUSTOM?

This would be at least better than what we have now.

Changed 10 years ago by deba

removing fixme and log

comment:14 in reply to: ↑ 13 ; follow-up: Changed 10 years ago by deba

Replying to alpar:

Replying to kpeter:

What about removing LEMON_FIXME

We should certainly revome LEMON_FIXME

and maybe LEMON_ASSERT_LOG, too, but keeping LEMON_ASSERT_ABORT (the default behavior) and LEMON_ASSERT_CUSTOM?

This would be at least better than what we have now.

I have uploaded the patch [7abfb55f1ecc] with such changes.

comment:15 in reply to: ↑ 14 Changed 10 years ago by alpar

Replying to deba:

This would be at least better than what we have now.

I have uploaded the patch [7abfb55f1ecc] with such changes.

It is in the main now. Thanks.

comment:16 follow-up: Changed 10 years ago by kpeter

Are there any open issues remained in this ticket?

All tools in assert.h seem to be clear. But what about error.h and the original questions of this ticket?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 10 years ago by alpar

Replying to kpeter:

All tools in assert.h seem to be clear.

And what about its usage? For example, its usage of LEMON_ASSERT in lemon/bits/base_extender.h is somewhat stange (IMHO it should use LEMON_DEBUG instead). Could anyone check it?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 10 years ago by kpeter

Replying to alpar:

And what about its usage? For example, its usage of LEMON_ASSERT in lemon/bits/base_extender.h is somewhat stange (IMHO it should use LEMON_DEBUG instead). Could anyone check it?

It seems to be good for me. LEMON_ASSERT is used in the copy constructors and operator=() functions of Red and Blue to check whether the given node of type Node actually red or blue. I think LEMON_ASSERT is reasonable here.

Maybe we could search for the other usage cases of these macros to check all of them.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 10 years ago by alpar

Replying to kpeter:

Replying to alpar:

And what about its usage? For example, its usage of LEMON_ASSERT in lemon/bits/base_extender.h is somewhat stange (IMHO it should use LEMON_DEBUG instead). Could anyone check it?

It seems to be good for me. LEMON_ASSERT is used in the copy constructors and operator=() functions of Red and Blue to check whether the given node of type Node actually red or blue. I think LEMON_ASSERT is reasonable here.

Now I feel brain damaged. LEMON_ASSERT is turned on by default, therefore it is to be used by convention only if it does not cause any noticeable running time overhead. But this usage does effects the running time, doesn't it?
Up to now I thought that LEMON_DEBUG was supposed to be used in such a case.

Btw, we haven't got bipartite graphs implementation in the hg repo, have we? Then why is this dead (i.e. unused and untested) piece of code there. It should be removed before the release. (I mean, from the 1.0 release branch. We can keep it in the main branch.)

Maybe we could search for the other usage cases of these macros to check all of them.

I've already done it. This is the only problematic usage (i.e. which breaks the rule I explained above.).

comment:20 in reply to: ↑ 19 ; follow-up: Changed 10 years ago by kpeter

Replying to alpar:

Btw, we haven't got bipartite graphs implementation in the hg repo, have we? Then why is this dead (i.e. unused and untested) piece of code there. It should be removed before the release. (I mean, from the 1.0 release branch. We can keep it in the main branch.)

I agree.

Now I feel brain damaged. LEMON_ASSERT is turned on by default, therefore it is to be used by convention only if it does not cause any noticeable running time overhead.
But this usage does effects the running time, doesn't it?

Yes, so maybe LEMON_DEBUG would actually better here. I just thought that these are necessary checks that cannot be omited, and so LEMON_ASSERT is reasonable, but I'm not sure.

What about replacing it with LEMON_DEBUG and note in the ticket about porting bipartite graphs that this class should be revised?

And what about error.h and the original questions of this ticket?

Changed 10 years ago by deba

comment:21 follow-up: Changed 10 years ago by deba

In the [305d03f9bcea] patch the exceptions are simplified. In my point of view, when we would like to signal error for users only input dependent runtime errors should be thrown as exceptions. Other way, we should use rather LEMON_ASSERT.

comment:22 in reply to: ↑ 20 ; follow-up: Changed 10 years ago by deba

Replying to kpeter:

Replying to alpar:

Btw, we haven't got bipartite graphs implementation in the hg repo, have we? Then why is this dead (i.e. unused and untested) piece of code there. It should be removed before the release. (I mean, from the 1.0 release branch. We can keep it in the main branch.)

I agree.

Now I feel brain damaged. LEMON_ASSERT is turned on by default, therefore it is to be used by convention only if it does not cause any noticeable running time overhead.
But this usage does effects the running time, doesn't it?

Yes, so maybe LEMON_DEBUG would actually better here. I just thought that these are necessary checks that cannot be omited, and so LEMON_ASSERT is reasonable, but I'm not sure.

I think, the DEBUG is the proper choice here. It is too time consuming to check anyway the nodes.

What about replacing it with LEMON_DEBUG and note in the ticket about porting bipartite graphs that this class should be revised?

And what about error.h and the original questions of this ticket?

Changed 10 years ago by kpeter

using DEBUG instead of ASSERT in graph extenders

comment:23 in reply to: ↑ 22 Changed 10 years ago by kpeter

Replying to deba:

I think, the DEBUG is the proper choice here. It is too time consuming to check anyway the nodes.

See [d91884dcd572].

comment:24 in reply to: ↑ 21 ; follow-up: Changed 10 years ago by alpar

Replying to deba:

In the [305d03f9bcea] patch the exceptions are simplified. In my point of view, when we would like to signal error for users only input dependent runtime errors should be thrown as exceptions. Other way, we should use rather LEMON_ASSERT.

I largely agree with the modifications in [305d03f9bcea], but I would keep the DataFormatError instead of IoError which should refer to real io related problems (like missing file, access rights problems etc), and should probably implemented in a later release.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 10 years ago by kpeter

Replying to alpar:

I largely agree with the modifications in [305d03f9bcea], but I would keep the DataFormatError instead of IoError which should refer to real io related problems (like missing file, access rights problems etc), and should probably implemented in a later release.

I agree. The simplification is good, but I also prefer keeping DataFormatError.

comment:26 in reply to: ↑ 25 Changed 10 years ago by alpar

Replying to kpeter:

I agree. The simplification is good, but I also prefer keeping DataFormatError.

I've created a IoError->DataFormatError search-and-replace changeset, see [8bed1c3a6080] in the attachment.

Another issue is however that it would be very nice to report the line numbers of the erroneous line somehow.

Changed 10 years ago by alpar

Changed 10 years ago by deba

Changed 10 years ago by kpeter

comment:27 follow-up: Changed 10 years ago by kpeter

[d901321d6555] contains improvements related to [f6899946c1ac] and it changes the parameter order in exception classes.

Could you please review it?

comment:28 in reply to: ↑ 27 Changed 10 years ago by alpar

  • Resolution set to fixed
  • Status changed from new to closed

Replying to kpeter:

[d901321d6555] contains improvements related to [f6899946c1ac] and it changes the parameter order in exception classes.

Could you please review it?

I like it therefore I close the ticket.

Changed 10 years ago by deba

Bug fix in exception throwing

comment:29 follow-up: Changed 10 years ago by deba

  • Resolution fixed deleted
  • Status changed from closed to reopened

The patch [7c796c1cf1b0] corrects a memory leak when an IoError? exception is thrown from LGF IO.

comment:30 in reply to: ↑ 29 Changed 10 years ago by alpar

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to deba:

The patch [7c796c1cf1b0] corrects a memory leak when an IoError? exception is thrown from LGF IO.

It went to the main branch.

Note: See TracTickets for help on using tickets.