On Tue, Feb 15, 2011 at 12:41:20AM +0100, Guillaume Melquiond wrote:
> On 2011/2/14, Mark de Wever wrote:
Hi Silene,
> > First I'm not sure what you're talking about, but I've a problem that
> > the code of the VALIDATE macro is broken due to Lua. The code works
> > properly in 1.4, works properly in 1.6, works properly 1.8, works
> > properly in trunk r48229, except when Lua happens to be in the
> > callstack.
>
> Why are you saying it is broken? Let me quote the documentation of the
> VALIDATE macro:
>
> * The macro to use for the validation of WML
> *
> * @param cond The condition to test, if false and exception
> is generated.
> * @param message The translatable message to show at the user.
>
> Right now, if the condition to test is false, the VALIDATE macro
> raises an exception, and the message is displayed to the user. Even
> when Lua is in the middle. So it is working fine in my book. Perhaps
> you have another semantic in mind for this macro, but I'm not psychic
> and I can't guess it.
Sorry I assumed you were more familiar with the VALIDATE macro and aware
it throws a twml_exception. In my opinion in the code of that exception
is clear that there are two messages to be shown. Below the code as in
1.8 since some of the comment was removed by you in [1].
<quote slightly reformatted for email inclusion>
/** Helper class, don't construct this directly. */
struct twml_exception
{
twml_exception(const t_string& user_msg
, const std::string& dev_msg);
/**
* The message for the user explaining what went wrong. This
* message can be translated so the user gets a explanation in
* his/her native tongue.
*/
t_string user_message;
/**
* The message for developers telling which problem was triggered,
* this shouldn't be translated. It's hard for a dev to parse
* errors in foreign tongues.
*/
std::string dev_message;
/**
* Shows the error in a dialog.
* @param disp The display object to show the message on.
*/
void show(display &disp);
};
</quote>
> > So the code works properly until it hits Lua, which seems to
> > be not exception-safe.
>
> Lua is exception-safe. There isn't a single exception that can cause
> it to crash, as long as you don't go directly modify the content of
> the src/lua directory.
In r48377 you reverted commit 48216 and fixed bug 17577. The bug report
states that; when setting an invalid terrain from Lua the game crashed.
Your fix was to change twml_exception from having no base class to have a
base class (indirectly) derived from std::exception.
In revision r48377 there are no modifications of mine in src/lua. This
means you were well aware that there are exceptions that can crash Lua. So
your claim above is not true. Lua can only handle exceptions derived from
std::exception and if it encounters such an exception it handles it. This
means that it is no longer possible to throw exceptions `through' Lua.
When twml_exception derives from std::exception Lua shows only a part of
the message, adding the second part could be fixed. Still the text in the
log is a regression from the dialogue. However Lua grabs and tries to
handle all exceptions deriving from std::exception. I filed a bug [2]
which shows the problem I mentioned in my previous mail. If code throws
and end_level_exception exception and that code suddenly goes through Lua
the end_level_exception is 'lost' and the game no longer works as
expected.
So I conclude that code that throws an exception which is caught by Lua
behaves like:
- catch all std::exceptions and `handle' them
- crash on all other exceptions
Since you mention that you are not psychic and complain the documentation
of VALIDATE is not clear enough I've some questions for you:
- Where in the source did you document that it's only save to use
std::exceptions? (I didn't check the source so it might be there.)
- Where in our coding standards [3] is this behaviour mentioned?
- When you added Lua to trunk you send an email [4] to this mailing list.
Where in this email did you mention this behaviour?
I replied with [5] to that email, quoted below the part that convinced me
it was better to move Lua to our tree. Unfortunately the quoted text does
not seem to work in practice.
<quote>
So, you have it: I imported the Lua code so that it gets compiled
along Wesnoth, hence simplifying the engine and fixing once and for
all all the future bugs related to C++/Lua interactions.
</quote>
> > So you broke a working feature with the current
> > Lua implementation and worse your changes also break the feature when
> > there is no Lua in the callstack.
>
> FUD.
All code that catches std::exception and is not aware of twml_exceptions
two messages will only show the (user_)message. If you wanted to derive it
from std::exception you should have left user_message and store the proper
combination of user_message and dev_message in message so the member
function what() would have returned a proper error message. (I did not
check whether this condition occurs, but the class had no base since I
only wanted it be be caught at a few places. But your change makes this
scenario likely; it already happened once, with Lua.)
> > But please take your own advice and fix the
> > Lua implementation so VALIDATE works again.
>
> VALIDATE works just fine; an exception is raised, and a message is
> displayed to the user. Sure, it doesn't display a GUI2 dialog box, and
> it doesn't cause the scenario to abruptly exit. But I don't call that
> a bug.
I leave the question whether or exiting wanted.
It is not only about VALIDATE it is about existing code which expects a
certain behaviour when throwing an exception, which may get broken when
Lua is moved in its call stack.
> But I don't care anymore: I'm now stripped from my commit rights
So instead of taking your own advice and fix your Lua implementation you
leave us with the bugs you introduced.
<snip a personal insult>
[1]
http://svn.gna.org/viewcvs/wesnoth/trunk/src/wml_exception.hpp?r1=44988&r2=44987&pathrev=44988
[2] https://gna.org/bugs/index.php?17743
[3] http://wiki.wesnoth.org/CodingStandards
[4] https://mail.gna.org/public/wesnoth-dev/2010-07/msg00068.html
[5] https://mail.gna.org/public/wesnoth-dev/2010-07/msg00071.html
--
Regards,
Mark de Wever aka Mordante/SkeletonCrew
_______________________________________________
Wesnoth-dev mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-dev
