Revise error.h and error_test.cc

[Peter Kovacs]

error_test.cc is not used as a test program in SVN and HG and it does not pass. So lemon/error.h and test/error_test.cc should be revised. At least in the HG repository.

[Balazs Dezso]

Reworking of assertions

[Alpar Juttner]

This change is a good idea. I already put it into the main branch [889d0c289d19].

But does it mean that we can close this ticket?

The exceptions should still undergo some review, shouldn't they?

[Peter Kovacs]

I have attached a patch file with improvements.

[Alpar Juttner]

This whole error infrastructure is still inherently wrong.

• Currently even the error_test.cc​ works very badly. It prints formal assert message but then continues running.
• I also noticed that in arg_parser.h and arg_parser.cc, there are several asserts but they do nothing in the default config.

For me it is very straightforward that an Error should be an exception and an ASSERT should assert independently from the configuration.

• Exceptions should be used when the algorithm is expected to be able to handle the problem (e.g. problems caused the bugs in the software are typically do not fall in this category .
• ASSERT should be used when some fatal error happened, typically caused by some bug in the code.

If want to have something that optionally checks different things during the execution, it must be called differently. This kind of tool might need an even finer control than switching on and off. Even in this case I see no reason to throw an exception instead of aborting (asserting).

[Balazs Dezso]

I think (accordingly to wikipedia) the assertions are that tools, which optionally checks the statements in runtime. In C the assertions can be turned off with the NDEBUG macro and in java has also ignorable assertion system. So I mean the thing, which independently from the compile settings stops the program execution, should be not called assert.

In my point of view, the assertions should express preconditions, postconditions and invariants, (and maybe variants). Because these are assumed to be true in a bug free program, the assertions can be switched off in a release program. The finer control would be delightful (maybe dependably on the overhead of the test), but in the current implementation the settings are limited to just the partial enabling in several code blocks of the program.

[Peter Kovacs]

Replying to alpar:

This whole error infrastructure is still inherently wrong.

• Currently even the error_test.cc​ works very badly. It prints formal assert message but then continues running.

It is a critical problem.

For me it is very straightforward that an Error should be an exception and an ASSERT should assert independently from the configuration.

But sometimes an assertion makes the code slower. That's why the concept of optional assertions seems better and more acceptable for me. So I agree with Balázs (deba).

ASSERT should be used when some fatal error happened, typically caused by some bug in the code.

What do you mean by fatal error?

I think e.g. using an index that is out of range is a fatal error, but it is typically not checked for efficiency reasons. What if I would like to check something like that in my algorithm, or check the correctness or optimality of some result (which must be optimal if there isn't any bug in the code) for safety especially in beta versions of the library. May I use ASSERT? I think, yes. However it is important to be able to switch these checks on/off, isn't it?

Even in this case I see no reason to throw an exception instead of aborting (asserting).

I agree with you. According to both of your concepts I see no reason to throw an exception instead of aborting. In my opinion exceptions are mainly used for handling unexpected but possible events (I mean events that can also be occur in a bug-free program), e.g. IO errors, overflow errors, memory errors.

After all, I do support switching assertions on/off, but I see no reason for an option that assertions throw exceptions.

[Alpar Juttner]

Replying to deba:

I think (accordingly to wikipedia) the assertions are that tools, which optionally checks the statements in runtime. In C the assertions can be turned off with the NDEBUG macro and in java has also ignorable assertion system. So I mean the thing, which independently from the compile settings stops the program execution, should be not called assert.

In my point of view, the assertions should express preconditions, postconditions and invariants, (and maybe variants). Because these are assumed to be true in a bug free program, the assertions can be switched off in a release program. The finer control would be delightful (maybe dependably on the overhead of the test), but in the current implementation the settings are limited to just the partial enabling in several code blocks of the program.

I accept this argument.

However, I still feel a major difference between the LEMON_ASSERTs in ArgParser and those are intended to check the validity of the Nodes at every NodeMap access. The first one does not affect the running time, thus should be turned on by default, but the second one should be turned off.

Also note that the assert in ArgParser check something which is very similar to a syntax error (i.e. a typo in writing something like a variable), while the second type of asserts above mainly check some kind of error in the logic of the algorithm.

Therefore we still need some kind of differentiation between the asserts.

[Alpar Juttner]

What do you mean by fatal error? I think e.g. using an index that is out of range is a fatal error, but it is typically not checked for efficiency reasons.

Yes, you are right. I only wanted to say two thing here.

1. There is no reason to trow an exception in an assert.
2. We should differentiate between the asserts according to that they do or do not affects the running time. I wanted to call the second group as assert and find some other name for the first one, but it really doesn't matter.

[Peter Kovacs]

Replying to alpar:

Also note that the assert in ArgParser check something which is very similar to a syntax error

And there is another important difference. Asserts in an algorithm is for checking the correctness of the LEMON code itself, but the asserts in ArgParser are for checking errors in the code that will use this class.

Replying to alpar:

1. There is no reason to trow an exception in an assert.

I agree with you.

2. We should differentiate between the asserts according to that they do or do not affects the running time.

I agree with you.

I wanted to call the second group as assert and find some other name for the first one, but it really doesn't matter.

I agree with Balázs in this question.

What about calling the second one as 'check()' (which is used in test programs)?

[Balazs Dezso]

I uploaded a patch which solves the next things:

• Assertion does not throw exception
• New LEMON_DEBUG macro for internal assertions

[Alpar Juttner]

Replying to deba:

I uploaded a patch which solves the next things:

• Assertion does not throw exception
• New LEMON_DEBUG macro for internal assertions

It is now in the main branch [407c08a0eae9]