Skip to content
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

Add logging support for linux. #79

Merged
merged 1 commit into from
May 20, 2015
Merged

Add logging support for linux. #79

merged 1 commit into from
May 20, 2015

Conversation

sand1k
Copy link
Contributor

@sand1k sand1k commented May 18, 2015

Added three macros:

#define JERRY_DLOG(...) 
#define JERRY_DDLOG(...) 
#define JERRY_DDDLOG(...) 

which take the same arguments as printf and log information with debug levels 1, 2, and 3 accordingly.

To build the engine with logging support pass LOG=ON option to make (could be passed only to linux targets):

make release.linux LOG=ON
make debug.linux LOG=ON

To enable logging, run the engine with --log-level option:
jerry --log-level 1 test.js
Supported values of --debug-level:

  • 0 - no logging
  • 1 - log level 1 (least detailed)
  • 2 - log level 2
  • 3 - log level 3 (most detailed)

By default, log messages are dumped to stdout. To specify log file, use --log-file option:
jerry --log-level 1 --log-file jerry.log test.js

@LaszloLango
Copy link
Contributor

Looks good, just a short comment. Warning and error messages of Jerry always should be printed to the stderr on Linux systems. This can be helpful when you pipe the output of the engine. There are some use cases when you want to separate the result of the evaluation from the errors and warnings.

@sand1k
Copy link
Contributor Author

sand1k commented May 19, 2015

@LaszloLango, it makes sense, I updated the pull request.

@@ -93,7 +94,7 @@ read_sources (const char *script_file_names[],

if (i < files_count)
{
printf ("Failed to read script N%d\n", i + 1);
fprintf (stderr, "Failed to read script N%d\n", i + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fprintf (stderr, 

Maybe we should introduce JERRY_ERROR or smth for error messages like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JERRY_ERROR would be a misleading name for this. Be careful with overusing the C/C++ macros, it can affect the readability of the source code negatively. If you want to create a macro for this, then name it to JERRY_LOG2STDERR or something more descriptive than JERRY_ERROR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look:

fprintf (stderr, "Error: wrong format of the arguments\n");
return JERRY_STANDALONE_EXIT_CODE_FAIL;

As for me, this is an error message, not the log one. If it's needed we may additionally pass error messages to the selected log stream.

P.S. JERRY_ERROR_MSG?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egavrin,
Sorry, I misunderstood what you were talking about. I thought you want something like this

   #define JERRY_ERROR(msg) fprintf(stderr,msg);

If error means error behavior (the engine exit with error code or crash), then the JERRY_ERROR is a good name, otherwise it'd be misleading.
UPDATE: JERRY_ERROR_MSG can be a better name, if it only prints the error message.

My opinion is still that errors and warnings always have to be written to the stderr, especially on Linux systems. Writing such messages to stdout is a bad practice and should be avoided. The debug log message is a different question, it can be written to the stdout and as I see this is how it currently works.
UPDATE: This blogpost summarize that quite well: http://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout

@egavrin egavrin added this to the Core ECMA features milestone May 19, 2015
@sand1k
Copy link
Contributor Author

sand1k commented May 19, 2015

Added the following marco:

JERRY_ERROR_MSG(...)
JERRY_WARNING_MSG(...)

which call fprintf (stderr, ...)

do \
{ \
fprintf(stderr, __VA_ARGS__); \
} while (0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "do { ... } while (0) trick is unnecessary here, because you have to close the function call with a semicolon anyway. Just remove the do-while and the semicolon after the fprintf call and it will be the same, but shorter and more readable.

#define JERRY_ERROR_MSG(...) fprintf (stderr, __VA_ARGS__)

@sand1k
Copy link
Contributor Author

sand1k commented May 20, 2015

@LaszloLango, fixed.

@LaszloLango
Copy link
Contributor

@sand1k, LGTM
@egavrin what do you think?

@egavrin
Copy link
Contributor

egavrin commented May 20, 2015

@LaszloLango looks good:)
@sand1k make push

JerryScript-DCO-1.0-Signed-off-by: Andrey Shitov a.shitov@samsung.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants