Thank you for your comments!
On Fri, Jun 17, 2011 at 1:02 PM, Mike Frysinger <[email protected]> wrote:
> On Friday, June 17, 2011 08:41:35 Jie Zhang wrote:
>> Any comments? If no one objects, I will commit it at the beginning of
>> the next week.
>
> the basic idea is fine, but some implementation issues ...
>
>> --- src/global/log-error.c (revision 1907)
>> +++ src/global/log-error.c (working copy)
>>
>> + char *msg_header;
>
> const
>
OK.
>> + if (level == URJ_LOG_LEVEL_WARNING)
>> + msg_header = "Warning";
>> + else if (level == URJ_LOG_LEVEL_ERROR)
>> + msg_header = "Error";
>> + else
>> + msg_header = "";
>
> so the msg_header is one of {"Warning", "Error", ""} ...
>
I thought this piece of code when I'm writing it but has no good idea
how to deal with it. I think urj_error_describe should only be called
for error messages. But there is one and only one place in UrJTAG
calling it for warning message. I don't know how to not do that for
that warning message. So urj_error_describe should only be called for
warning or error. Maybe I should use an assert under the else branch?
What do you think?
>> - snprintf (msg, sizeof msg, "%s:%d %s() %s: %s %s",
>> + snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s %s",
>> + msg_header,
>>
>> + snprintf (msg, sizeof msg, "%s: %s:%d %s() %s: %s",
>> + msg_header,
>> + urj_error_state.file, urj_error_state.line,
>
> but you always use "%s: " with msg_header, even when it's "". so perhaps the
> ": " should be moved into msg_header and the format string changed to just
> "%s" rather than "%s: ".
>
Maybe. Depending on the way we choose for the former question. If we
use an assert, leaving ':' here makes the pattern a little more
structural.
>> - urj_log (URJ_LOG_LEVEL_NORMAL, "Error: %s\n", urj_error_describe());
>> + urj_log (URJ_LOG_LEVEL_ERROR, "%s\n",
>> + urj_error_describe(URJ_LOG_LEVEL_ERROR));
>
> there seems to be a lot of this style now. this brings up two questions ...
>
> do we ever use urj_log() without a trailing new line ? seems like we should
> move that into urj_log() itself.
>
I don't know the answer.
> many (most? all?) of our logs are of the form:
> "xxxxxx: %s\n", ..., urj_error_describe()
> so perhaps it'd make sense to add a bit to the first parameter of urj_log() so
> that it'll automatically append the error description. think perror().
>
> and along those lines, if many times we are simply displaying the error, then
> perhaps add a helper func like urj_log_simple(level) which expands into:
> urj_log(level | URJ_LOG_APPEND_ERROR, "");
>
Actually, only a small portion of our logs are of that form. Most just
look like urj_log (level, "log message", ...) and urj_warning to
immediately print out the message when the error happens.
I'm not in favor of the error reporting system currently used in
UrJTAG. It's too complicated for me. For example,
1. I cannot find a suitable errnum for some error messages in ICE-100B
cable. I thought adding a new errnum like URJ_ERROR_ICE_100B. But does
that means we need a number for each cable driver?
2. urj_error_set/urj_error_describe is difficult to use. Sometimes I
don't want my error message to show file/line/function before it. It's
not useful to user and makes the error message read strange. But it's
not easy to disable this.
3. currently the error messages are handled using two ways. One is
output immediately. The other is to store it and output late. I don't
know when to use which, or we should always use the latter, but there
are some many code using the former now.
Maybe we should take some efforts to simplify it and make it more
consistent throughout UrJTAG.
Regards,
Jie
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
