Revise debug modes and exceptions

[Alpar Juttner]

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

[Alpar Juttner]

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?

[Peter Kovacs]

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.

[Peter Kovacs]

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.

[Alpar Juttner]

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.

[Peter Kovacs]

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.

[Alpar Juttner]

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?

[Balazs Dezso]

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().

[Peter Kovacs]

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?

[Alpar Juttner]

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.

[Balazs Dezso]

removing fixme and log

[Balazs Dezso]

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.

[Alpar Juttner]

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.

[Peter Kovacs]

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?

[Alpar Juttner]

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?

[Peter Kovacs]

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.

[Alpar Juttner]

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.).

[Peter Kovacs]

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?

[Balazs Dezso]

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.

[Balazs Dezso]

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?

[Peter Kovacs]

using DEBUG instead of ASSERT in graph extenders

[Peter Kovacs]

Replying to deba:

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

See [d91884dcd572].

[Alpar Juttner]

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.

[Peter Kovacs]

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.

[Alpar Juttner]

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.

[Peter Kovacs]

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

Could you please review it?

[Alpar Juttner]

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.

[Balazs Dezso]

Bug fix in exception throwing

[Balazs Dezso]

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

[Alpar Juttner]

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.