-
Notifications
You must be signed in to change notification settings - Fork 688
Implement IO port API #1201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement IO port API #1201
Conversation
|
@pfalcon, @sergioamr please check the Zephyr port. I hope my speculative update work as it should. |
|
Note: after this PR, we will change all the printf calls inside the engine to the related port API function ( |
|
cc: @jiangzidong |
jerry-core/jrt/jrt.h
Outdated
|
|
||
| #define JERRY_ERROR_MSG(...) jerry_port_errormsg (__VA_ARGS__) | ||
| #define JERRY_ERROR_MSG(...) jerry_port_log (JERRY_LOG_LEVEL_ERROR, __VA_ARGS__) | ||
| #define JERRY_WARNING_MSG(...) JERRY_ERROR_MSG (__VA_ARGS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the warning use warning level?
a354d78 to
bcfa39e
Compare
|
Will check it in my morning tomorrow :) |
|
It will bring convenience to porting work. Thanks |
targets/esp8266/user/jerry_port.c
Outdated
| va_start (args, format); | ||
| // TODO, uncomment when vfprint link is ok | ||
| //count = vfprintf (stream, format, args); | ||
| // vfprintf (stdout, format, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use /* */ comment style
|
LGTM after the comment fixes. |
033324c to
adf2301
Compare
|
LGTM |
adf2301 to
e8e11b3
Compare
| do \ | ||
| { \ | ||
| if (lvl <= jerry_debug_level && jerry_log_file) \ | ||
| if (lvl <= jerry_debug_level) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this check should not happen here but in the port implementation (if that impl cares about it). Likewise, jerry_debug_level shouldn't even be defined in jerry.c. Everything JERRY_LOG should do is just pass things on to jerry_port_log implemented in the port for it to decide what to do with it (as lvl is passed on as well).
In some sense, it should work similarly to the "fatal" logic. As far ar jerry-core is concerned, there is only a single jerry_port_fatal function to be called. However, the default port implementation adds some extras to it, namely the "abort_on_fail" functionality. However, that's a port-specific thing. Therefore, the static bool abort_on_fail variable is defined in jerry-port-default-fatal.c, and accessor methods jerry_port_default_set_abort_on_fail and jerry_port_default_is_abort_on_fail in jerry-port-default.h.
I can imagine a similar scenario with the logs. There could be default port-specific accessors, e.g., jerry_port_default_set_log_level and jerry_port_default_get_log_level in jerry-port-default.h and their implementation (along with any variables needed for that logic, say debug_level or log_level) in jerry-port-default-io.c. And then the line (void) level; /* default port implementation ignores the log level */ will go away (actually, the presence of this line already signals the problem that log levels are not checked at the right place, IMHO).
Additonal notes:
JERRY_D[D[D]]LOGmacros callJERRY_LOGwith 1, 2, and 3 as level parameters. This does not seem right in the light of the new log level enum.- There are still some
printfs in the code (e.g., jrt-fatals.c, js-parser.c, js-parser-util.c, ecma-builtin-global.c, jmem-poolman.c, jmem-heap.c, js-parser-statm.c), even if mostly macro-guarded. When will these be rewritten to call the_logand_consolefunctions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry, missed the "Note: after this PR," part. Cancel my last Q.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with the first part. JERRY_D[D[D]]LOG macros are all on debug level. There where no multiple debug levels in the proposed (and accepted) IO port API proposal (see: #964).
I think moving the debug level to the port is might be a good idea. I'll try to do it in a followup work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifdef JERRY_ENABLE_LOG
#define JERRY_LOG(lvl, ...) \
do \
{ \
if (lvl <= jerry_debug_level) \
{ \
jerry_port_log (JERRY_LOG_LEVEL_DEBUG, __VA_ARGS__); \
} \
} \
while (0)
#define JERRY_DLOG(...) JERRY_LOG (1, __VA_ARGS__)
#define JERRY_DDLOG(...) JERRY_LOG (2, __VA_ARGS__)
#define JERRY_DDDLOG(...) JERRY_LOG (3, __VA_ARGS__)
#else /* !JERRY_ENABLE_LOG */
#define JERRY_DLOG(...) \
do \
{ \
if (false) \
{ \
jerry_ref_unused_variables (0, __VA_ARGS__); \
} \
} while (0)
#define JERRY_DDLOG(...) JERRY_DLOG (__VA_ARGS__)
#define JERRY_DDDLOG(...) JERRY_DLOG (__VA_ARGS__)
#endif /* JERRY_ENABLE_LOG */This is current status. JERRY_D[D[D]]LOG macros expand to JERRY_LOG (]0-3], ...) depending on JERRY_ENABLE_LOG settings. The first parameter should be the debug level you just mentioned, not integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akosthekiss, as I said. I'll do it in a follow up work. Don't worry. It's on my list to clean this up.
|
tested branch on my arduino101. LGTM |
|
@sergioamr, thanks |
Related issue: jerryscript-project#964 Implemented the IO API of Jerry ports. Removed log file from API level. The port implementation should define the destination of log messages. JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
e8e11b3 to
fa94c67
Compare
Related issue: #964
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com