[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150417154711.GA15757@openwall.com>
Date: Fri, 17 Apr 2015 18:47:11 +0300
From: Aleksey Cherepanov <lyosha@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Improving Johnny
On Fri, Apr 17, 2015 at 09:30:20AM -0400, Mathieu Laprise wrote:
> On Fri, Apr 17, 2015 at 7:50 AM, Aleksey Cherepanov <lyosha@...nwall.com>
> wrote:
>
> > Mathieu,
> >
> > On Tue, Apr 14, 2015 at 02:40:38PM -0400, Mathieu Laprise wrote:
> > > First, I reviewed the code to see if the string were marked as
> > > translatable. Almost all of them were. I found a problematic case in
> > > MainWindow::showJohnError. There were two functions in #define and their
> > > strings didn't register as translatable in the .ts file.
> >
> > I think you talk about
> >
> > #define C(code, text) case QProcess :: code: message = tr(text); break;
> > #define P(code) C(code, "Problem with john: " # code);
> > switch (error) {
> > C(FailedToStart,
> > "John failed to start. "
> > "Check your Path to John setting. "
> > "Check permissions on respective file.");
> > C(Crashed, "John crashed.");
> > P(Timedout);
> > P(WriteError);
> > P(ReadError);
> > P(UnknownError);
> > default:
> > message = tr("There is a problem. Johnny could not handle it.");
> > }
> > #undef P
> > #undef C
> >
> > C and P are not functions, they are macros. So they are expanded
> > before compilation and tr() is called for each string. "their strings
> > didn't register as translatable in the .ts file" - is it your
> > assumption? Did you try it? Or are they not registered as translatable
> > in some qt's tool? Please give more details, I'm curious.
> >
> > I know that your macros are writing something like this for each case
> before the program is compiled:
> case QProcess::Timedout:
>
> message = tr("Problem with john Timedout");
>
> break;
>
>
> I agree that it's weird because it'll, at some point, become the same as
> writing it directly in the .cpp. However, it's not an assumption,I tried it
> and it didn't work with translation. All strings except those in this
> function were in the .ts file which is generated by calling lupdate. Even
> the one that were not related to enum like "John failed to start, check
> your path etc.." didn't. Maybe it ignores macros, I don't know, I didn't
> try to fix the macros as the code needed a refactoring anyway.
Good!
> > Also you've changed the behaviour:
> >
> > + message = tr("Problem with john #") + QString::number(error);
> >
> > So johnny prints error's number while the code with macros showed
> > error's name. Do you understand why? Please explain.
> >
> > I'd say it is a regression. Though the piece of code is
> > overcomplicated and with macros. Macros are a bad style for C++. So
> > I think the regression is ok as an intermediate step.
> >
> > Thanks
> >
> Yeah, sorry I misinterpreted the two macros. Regarding why it's doing that,
> it's because it's writing the code that I wrote previously in this message.
> For such a simple function that write 6 error messages, if we want to keep
> good readability of the code and 100% translatable error messages, let's
> just separate the switch case with 4
> cases(Timedout,WriteError,ReadError,UnknownError) into different
> cases/error messages.
It is good that you don't stall overthinking things. There is no real
problem with misinterpretation while you're short on time. Though we
will expect quite high quality of code later because Johnny is not
considered as hard/big task. You may and you will break things but
you'll have to recognize these moments.
Separate messages should be good because we may need to change them to
more useful descriptions of the problems (though we should understand
the codes, for that it will be useful to have tests to trigger all the
codes). Though it is not important if users don't face it at all.
Some notes on macros:
You may pass a part of code through `gcc -E -` to see expanded macros:
switch (error) {
case QProcess :: FailedToStart: message = tr("John failed to start. " "Check your Path to John setting. " "Check permissions on respective file."); break;
;
case QProcess :: Crashed: message = tr("John crashed."); break;;
case QProcess :: Timedout: message = tr("Problem with john: " "Timedout"); break;;;
case QProcess :: WriteError: message = tr("Problem with john: " "WriteError"); break;;;
case QProcess :: ReadError: message = tr("Problem with john: " "ReadError"); break;;;
case QProcess :: UnknownError: message = tr("Problem with john: " "UnknownError"); break;;;
default:
message = tr("There is a problem. Johnny could not handle it.");
}
'#' in macro is a conversion to a string literal (identifier -> string
constant):
$ gcc -E -
#define a(b) #b
a(QWER)
END
^D (Control-D; or ^Z on Windows)
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "<stdin>"
"QWER"
END
$ gcc -E -
#define a(b) "begin" #b "end"
a(MyErrorCode)
END
^D (Control-D; or ^Z on Windows)
[...]
"begin" "MyErrorCode" "end"
END
I've typed END to mark end of input and end of output.
Then "begin" "MyErrorCode" "end" is equal to "beginMyErrorCodeend".
Maybe I have to not put a space after # to make it look more like
unary operator.
Thanks!
--
Regards,
Aleksey Cherepanov
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
