COIN-OR::LEMON - Graph Library

Opened 11 years ago

Closed 11 years ago

#28 closed defect (fixed)

Revise error.h and error_test.cc

Reported by: Peter Kovacs Owned by: Balazs Dezso
Priority: critical Milestone: LEMON 1.0 release
Component: core Version: hg main
Keywords: Cc: Balazs Dezso
Revision id:

Description

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.

Attachments (3)

assertion_rework.bundle (4.8 KB) - added by Balazs Dezso 11 years ago.
Reworking of assertions
lemon_assert.patch (9.1 KB) - added by Peter Kovacs 11 years ago.
assert_rework.hg (19.1 KB) - added by Balazs Dezso 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by Alpar Juttner

Owner: changed from Alpar Juttner to Balazs Dezso

Changed 11 years ago by Balazs Dezso

Attachment: assertion_rework.bundle added

Reworking of assertions

comment:2 Changed 11 years ago by Balazs Dezso

Resolution: fixed
Status: newclosed

comment:3 in reply to:  2 Changed 11 years ago by Alpar Juttner

Resolution: fixed
Status: closedreopened

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?

Changed 11 years ago by Peter Kovacs

Attachment: lemon_assert.patch added

comment:4 Changed 11 years ago by Peter Kovacs

I have attached a patch file with improvements.

comment:5 Changed 11 years ago by Alpar Juttner

Cc: Balazs Dezso added
Priority: majorcritical

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

comment:6 in reply to:  5 ; Changed 11 years ago by 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.

comment:7 in reply to:  5 ; Changed 11 years ago by 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.

comment:8 in reply to:  6 Changed 11 years ago by 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.

comment:9 in reply to:  7 ; Changed 11 years ago by 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.

comment:10 in reply to:  9 Changed 11 years ago by 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.

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

Changed 11 years ago by Balazs Dezso

Attachment: assert_rework.hg added

comment:11 Changed 11 years ago by Balazs Dezso

I uploaded a patch which solves the next things:

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

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

Resolution: fixed
Status: reopenedclosed

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]

Note: See TracTickets for help on using tickets.