So I just just created the errorchecking branch, where I'm adding the
error checking code mentioned some days ago.

Currently I'm just replacing dAASSERT() and some dUASSERT() with
dCheck*(), taking care to not leak anything. I'm using camelCase instead
of ALLCAPS because it's more readable and easier to type. dAASSERT()
(and possibly dUASSERT too) will naturally disappear, and dIASSERT()
will be moved to the private header (error-private.h).

There are 3 behaviors:
* dNOERRORCHECK: as the name implies
* dFATALERRORS: just abort (will probably change into the current
d*ASSERT() behavior)
* dEXCEPTIONS: throws an exception
* default: register the error with dSetLastError(), usually returns
either an invalid or a safe value.

Currently only ode.cpp was updated, so I would like to ask for comments
about the implementation. Specially about how detailed the error
messages should be. Is it worth to have an extra descriptive string,
from the context where the error ocurred?

PS: As this will touch a ton of lines, I started adding modelines to the
end of the files (currently only tested on emacs). They should work with
mainstream editors (although most require them in the top of the files).
Would anyone oppose modelines on the top of the file (so it will work
with vim, kate, gedit, etc)? Does VS have anything similar?

--
Daniel K. O.
"The only way to succeed is to build success yourself"

Currently asserts are customizable.
User can assign its own assert handler and what he likes: throw and
exception, call abort or assign "last error".
Why are all these complications you started for?

Oleh Derevenko
-- ICQ: 36361783

1) It is incorrect to replace ASSERT with return 0. If code does not expect
NULL-pointer it will not do any good to just return because further code in
caller may not be able to handle that invalid return.
Each such assert must be checked. You must verify all the places the
function could be called from and make sure the caller can handle that
invalid result. Do you realize what amound of work is that? That is
monotonous work that makes you tired very soon. Can you predict how many
errors you'll make?
2) ASSERT is written in all uppercase to distinguish it from ordinary
functions. It shows you immediately, that ASSERT is not a function but a
macro and its behavior may change or it may be removed from code. Changing
case to function-like makes code much more unclear.
3) Returns from inside of the function are the bad style. They lead to
potential bugs. Especially dangerous are returns hidden in macros and
especially when those macros look like functions.

My resume:
You will not achieve anything by just changing one macro to another. You
need to distinguish real asserts from false asserts and change false asserts
to error return and error handling in all the callers. But this is huge
amount of work which requires analysis of whole codebase and I doubt you are
such a maniac to do it. So, please do not change anything.

Oleh Derevenko
-- ICQ: 36361783

I'm a longtime VS user and I haven't heard of such a feature, though
that's no guarantee. My preference is that editor-specific
meta-information not be added to the files at all--top or bottom.

Dave

2008/3/22, Oleh Derevenko <o...@eleks.lviv.ua>:

They are non-recoverable.

For an application with limited interactivity, like a game, you don't
need it. You design and tune everything so it always work (heh... you
wish).

For more interactivity you need to fail gracefully.

Ok, this is the only "recoverable" way, with the current asserts. But
only for an exception-enabled build. And exceptions are always slower
(although we can debate about efficiency when things fail...).

That's the only way to use current asserts.

How can you set the error code when all the handler gets is a string
describing the error?

Because I need them for an interactive physics environment, where the
user is supposed to play with all of ODE. I don't want it aborting,
throwing exceptions to C glue code (script language bindings in C). If
I have to write checks for everything (say, to catch every exception
and translate into proper error handling, hoping nothing inside ODE
leaks because of the throw), I seems more useful to have into the
core, so others can benefit.

--
Daniel K. O.
"The only way to succeed is to build success yourself"

2008/3/22, Oleh Derevenko <o...@eleks.lviv.ua>:

Geez, thanks for the motivation.
Don't worry, I'm carefully checking the codepath.

Note that even if I miss something, it won't be worse than the current
implementation. It's actually a superset of the asserts we are using.

It's more a matter of taste. On glib, similar macros are lower-case
(g_return_if_fail()). Note that I'm encoding more than a single word
in the macro name, it's easier to read when there are some separators.

I'm being extra careful.
Note that it causes problem with just one of the possible semantics
(dSetLastError()), which you don't seem fond of. The current code is
just as broken (actually, more broken, as I added some more argument
tests that were missing).

I'm not changing one macro to another. If I were, I would just rewrite
dAASSERT(). :P
I'm carefully adding semantics, one piece at time. It might take long
to finish, but it's supposed to be always consistent.

I won't even push to get this accepted into trunk, if there's really
opposition. But I hope that by "do not change anything" you don't mean
to not play around the branch.

--
Daniel K. O.
"The only way to succeed is to build success yourself"

2008/3/22, Dave Grundgeiger <dave.grundgei...@codenouveau.com>:

Geez, it's always those MS devs that spoil the fun. Just because you
guys don't have shiny toys we are not allowed to play with them. :P

It would help improving code formatting - a problem that was mentioned
many times, but no one has balls to apply a reformatting tool to fix
all the code base. Modelines actually go a step further, making sure
the formatting stays the same.

But it's fine, I can just remove the extra comment lines at the end of
the file, if that really bothers VS users. Formatting policy is not
the primary objective anyways (for now).

PS: for those wondering what this is about,
http://www.gnu.org/software/emacs/manual/html_node/emacs/Specifying-F...

--
Daniel K. O.
"The only way to succeed is to build success yourself"

I don't care, as long as they go on the end. Have the VS users who
actually care remove them, if they're so persnickety :-)

Btw: I use gvim on Windows at times, and Visual Studio at times. Visual
Studio takes a different approach to "common formatting:" everybody in
the organization can import the same settings file, and then all the
code you write will have the same settings. Can't say one is better or
worse than the other. (And I bet you could write a Visual Studio Macro
that would parse the mode lines).

Sincerely,

jw

... which wouldn't work on the Express versions (no macro support).
--
gl

OK. Let me describe my opinion in more details.

What do we have now
---------------------------------
We have several macros which call dDebug/dMessage/dError. These functions
either invoke a user provided callback or print the message and call
exit/abort/nothing. Not all the asserts currently are real asserts. Some of
them just substitute error checking (what is wrong and what's got to be
fixed).
From those macros we have:
1) dIASSERT is the real assertion check. It has to remain where it is used
propertly and replaced with other methods where it is used for wrong
purposes. It has to be used through whole codebase. To do it, you have to
have good understanding of whole library structure and of logic used in it
at all the levels.
2) dUASSERT is the user assert macro. Actually, this macro is the nonsense
and should be removed. User can't do anything wrong to trigger the assert.
If library allowed that, it's the fault of library, not the fault of the
user. Normally it has to be replaced with parameter validation and error
return.
Parameter validation must be used only on external entry points in public
API. You only need to validate input values that come from outside of
library. If all external entries are secured, there should never appear
invalid parameters in internal calls. If that happens that is dIASSERT.
3) dAASSERT is just a subclass of dUASSERT and should be removed as well.
4) dDEBUGMSG is not an assert but just a logging replacement. It should be
used to trace some debug information where it is difficult to view it with
debugger and should not influence the execution flow.
If dIASSERT is placed in the code correctly and if dUASSERT is replaced with
error checking that would be sufficient error checking measures.

What do I see in your changes
---------------------------------
1) An option to ignore all the checks.
Well, that's one of the options which we currently have. Though it is not
correct to ignore the parameter validation. You can ignore asserts, but
parameter validation on external entry points must be performed always. The
library is a closed entity and it can't trust any external data whether it
is a release build or not. If you trust the validity of external input you
agree to be guilty for the faults in external code which may result in
feeding you corrupted input data.
2) An option to call system assert().
This is even less than what we have now. Because now we have a customizable
asserts. And I personally use that feature in my project. I also do call
abort() but I do it a special way from my own assertion failure handler.
3) An option to thow an exception or SetLastError(dErrNoError) if condition
is true.
First of all, let me mention that setting LastError on succes is not the
best approach. All unixes obey the rule (unfortunartely, Microsoft were not
wise enough to follow it in WinAPI) that errno is assigned on failure only.
A successful call preserves last error code. The intent of this rule is to
keep error code intact during exit from the call stack on failure.
Imagine, in a middle of the invocation stack you open a file or allocate
some memory and call child function after that. When that lower level
function fails you have to free temporary memory or close the file before
you pass the error through to the upper levels. But memory deallocation of
file closure will succeed. And if they assigned EOK they would have erased
the last error code which you have to pass through.
Now, about exceptions themselves. As I can see you have several exception
codes that classify the nature of the fault. But truly speaking that is just
slightly better from what we have now. Since all the assertion checking
macroes are currently customizable you can assign your own handler, pack
source line and file name along with assertion kind into and exception and
throw it. Both your exceptions and exceptions that might be currently
generated from assertion handler are useles for the program. They are not
suitable for carrying out any automatic handling in client code. They are
only worth for presenting them to user in a bessage box. Imagine, you call
dQuickStep() and get  exception with dErrInvalidBody. Well, and what? How is
this better from getting exception with the string "assert in file aaa.cpp
in line 1234"?
Also, please remember that just raising an exception is not enough. If you
raise the exception you must look through all the possible call stacks and
put try ... catch() { free(); throw; } to deallocate all the temporary
resources (memory, handles, objects) that might be allocated in callers.
4) An option to set LastError and return (with void or with invalid value)
in case of error.
The remark about assignment of last error on success applies here as well. I
also have forgotten to mention that assignment of last error on success is
the unnecessary waste of CPU resources. Returning a boolean would be much
cheaper.
Next, setting LastError and returning void is a weird approach. People will
not understand it. I understand that you do not want to change function
prototypes, but you should change them if you want to achieve any
improvement. If function has a void return it implies that it can't fail.
And it looks very unnatural to call a void function and check last error
after that.
By the way, how do you plan to check last error after void functions?
Depending on compilation mode they may not return last error at all. They
may throw an exception or just ignore the error. Adding checking for last
error after function calls would be an unnecessary waste of code in these
modes. Are you going to add one more macro?
Returning invalid value (false status) is the most correct approach. But
just like with exceptions it is not enough to just return NULL. You have to
check the possible callers and make sure that they are prepared for the
possibility of invalid return and that they cancel their job, free resources
that they might have allocated, and return an invalid result on their own.
And then you have to check all callers of possible callers to make sure they
handle invalid return correctly. And the callers of the callers of the
callers. And so on until exit from the library into client code where our
responsibility ends. Many of public APIs are not designed for error
handling. They do not have execution status returns. You will have to change
them if you do not want to just make things even worse.

By adding possibility of invalid returns or exceptions you create new rules
which will have to be obeyed for future code development. Let's imagine that
the miracle happens and you implement correct error handling in all the
code. But who will watch for future changes and who will tell all the
developers that they have to check for lasterror after each void call, and
that they have to put try..catch() { free/delete; throw; } in the code
because each function can throw an exception? Here all the developers are
uncontrollable. There is no management. It will be a complete mess.

Also, parameter validation is a good thing (on the public API only!!!) but
do not hide "if" statements in the macros. It is very unclear and confusing.
And it is hard to step through it in debugger. Do you remember we agreed to
limit macro usage? If you do parameter validation, put conditional
statements explicitly in the code and only hide failure branch in the macro
(because it may change).

Also, about my statement that returns from inside of the functions are the
bad style and your response that you are checking everything carefully.
Even if you watch for returns from inside of the functions, others may miss
them. It is common source of bugs if you add something at the end of large
function and do not notice that your code may be skipped because somewhere
in middle of it there is a burried return statement.

To conclude:
1) You can implement correct error handling, but only one of them. Error
returns and exceptions are quite incompatible and supporting them both will
make code too heavy.
2) Replacing all the assertions with return NULL or throw Exception is
incorrect. Assertions are the assertions and parameter validation is the
parameter validation. They can't be substituted with each other.
3) It is incorrect to allow eliminating all the validations in release
build. Especially external input validations. And also there may be the
needed to have special assertions kind which can be left active in release
build as welll. There can be places where functions should normally never
fail and adding error returns is difficult and/or would make code
unnecessarily complicated. So placing release asserts is the best choice for
these cases. A great example is the call to pthread_cond_signal() or
pthread_mutex_unlock().
4) You should not restrict yourself in changing function prototypes. Because
if something is not designed for some purpose it is not designed for that
purpose. It is better to change it once and live happily ever after than to
struggle with it and try to conform to it all your life with no success.
5) You do not show enough understanding of the complexity and depth of the
problem (that's just my personal opinion based on what I've seen in your
initial design) and if you continue the development this way and the changes
will be accepted that will be a disaster for the library. I suggest you
spending more time on planning the apporaches and developing base
instruments (macroes/functions) you will use. Because these are the most
important things and a little mistake at the beginning can make all your
work worthless and changes unusable.You have to have a clear image in your
mind of how it will work on all levels and in all variants in whole library
before you start implementing anything.

Oleh Derevenko
-- ICQ: 36361783

...

read more »

It is not the matter of taste. Macros must obey different naming conventions