Update jerry-port and jerry-ext #4907

dbatyai · 2021-12-15T14:37:59Z

Notable changes:

Updated and the port API interface, new functions have been added
and some have been changed. The port library is now cleaned up to
not have any dependency on jerry-core, as it should be. The port library
is now strictly a collection of functions that implement
embedding/platform specific behavior.
The default port implementation has been split for windows and unix.
Implemented port functions have been categorized and reorganized,
and marked with attribute((weak)) for better reusability.
External context allocation has been moved to the port API instead
of a core API callback. The iterface has also been extended with a
function to free the allocated context. When external context is
enabled, jerry_init now automatically calls the port implementation
to allocate the context and jerry_cleanup automatically calls the port
to free the context.
jerry_port_log has been changed to no longer require formatting to
be implemented by the port. The reason beind this is that it was vague what
format specifiers were used by the engine, and in what manner. The port
function now takes a zero-terminated string, and should only implement
how the string should be logged.
Logging and log message formatting is now handled by the core jerry library
where it can be implemented as necessary. Logging can be done through a new
core API function, which uses the port to output the final log message.
Log level has been moved into jerry-core, and an API function has
been added to set the log level. It should be the library that
filters log messages based on the requested log level, instead of
logging everything and requiring the user to do so.
Module resolving logic has been moved into jerry-core. There's no
reason to have it in the port library and requiring embedders to
duplicate the code. It also added an unnecessary dependency on
jerry-core to the port. Platform specific behavior is still used through
the port API, like resolving module specifiers, and reading source file
contents. If necessary, the resolving logic can still be overridden as
previously.
The jerry-ext library has also been cleaned up, and many utility
functions have been added that previously were implemented in
jerry-main. This allows easier reusability for some common operations,
like printing unhandled exceptions or providing a repl console.
Debugger interaction with logged/printed messages has been fixed, so
that it's no longer the port implementations responsibility to send
the output to the debugger, as the port should have no notion of what a
debugger is. The printing and logging functions will now pass the
result message to the debugger, if connected.

lygstate · 2021-12-26T07:11:43Z

Any thought about atomic-port for different platform?
Some CPU have atomic support, but some won't have it and we will use Locking to simulate atomic.

dbatyai · 2022-01-10T14:00:16Z

I think it makes sense to add port functions for atomics, however I think it would be better as a separate patch.

jerry-port/win/jerry-port-win-fs.c

lygstate · 2022-01-13T14:01:05Z

jerry-core/include/jerryscript-port.h

 */

 JERRY_C_API_END

 #endif /* !JERRYSCRIPT_PORT_H */
+
+/* vim: set fdm=marker fmr=@{,@}: */


Does this are necessary?

It's not necessary, however it does not hurt anyone, and may be helpful for some.

Can this be in config file?
as jerryscript-core.h also need this.

I almost wrote that we don't have anything like that in the current code base. Unfortunately, it is not completely true, as jerryscript-core.h already contains such a line. It has crept in somehow. IMHO it should not have.

I don't really see what the fuss is about.

It is a file specific comment for a specific editor. We have other options set here and there for other IDE-s as well. Bunch of git-ignores for a lot of editors, generated compile_commands.json to enable code completion in vscode, etc.

It is a simple comment, it's not intrusive, it has no effect on anyone unless they are using vim, in which case it is actually helpful.

Bunch of git-ignores for a lot of editors

I'm not sure what you mean by that. Calling gitignore an IDE/editor-specific file would be an overstatement. If you mean that gitignore contains lines that mention IDE-specific files, well, then yes. But that means two things: those files are not included in the code base & those details are kept at a single place (i.e., in the gitignore file).

generated compile_commands.json to enable code completion in vscode, etc.

Again, these are kept in separate files, not mixed into the sources.

As for vim, it should be possible for that editor too to keep its settings in a single file. I'm not a user of vim but did some research and found several options:

https://vim.fandom.com/wiki/Project_specific_settings

https://github.com/LucHermitte/local_vimrc

https://github.com/embear/vim-localvimrc

And regarding "what the fuss is about", I completely agree with https://stackoverflow.com/questions/456792/vim-apply-settings-on-files-in-directory/456889#456889

DRY: with modelines, a setting needs to be repeated in every file, if there are too many things to set or tunings to change, it will quickly become difficult to maintain, moreover, it will require the use of a template-expander plugin (which you should consider if you have several vimmers in your project).
Not every one uses vim to develop. I don't want to be bothered by other people editor settings, why should I parasite theirs?
It's easier to ask vimmers to install a same plugin, instead of asking them to copy-paste, and maintain, the same lines in their .vimrc
The settings can be saved with the other project files (cvs/svn/git/whatever)

I'm not calling gitignore IDE specific, I'm pointing out that our gitignore is already bloated with editor specific ignore lines. These wouldn't be necessary, as they don't pertain to the code base, and people could ignore them locally in their repository, depending on what they do or do not use. Nevertheless, we keep it in the repository to help developers and make their life easier.

However back to the point.
This modeline is not needed for every file in the source tree. What this modeline does, just to make things clear, is it defines how folds can be created for doxygen comment groups, so that they can optionally be collapsed, to make reading the file easier. It isn't a typical 'editor setting', that configures the editor in just the right way, like setting tab width etc. These directives are tied to doxygen comment groups, and don't need any maintenance as these don't change.

The reason I added them specifically to these headers is that these files contain a lot of text, and will contain more in the near future. Allowing to collapse groups makes reading the file a lot easier.

Think of it this way. If I were to add a hard-to-understand line of code, I would add a line of comment describing it.
That comment would contain useful information which the developer can use to understand the code more easily. This comment does exactly the same, it contains helpful information for reading the file, with the one exception, that it is not in some textual human language. Even if someone uses vim, they are not obligated to use it, they can decide whether they want to parse modelines or not.

For the majority of users this comment will affect nothing. It requires no maintenance, it is not needed to be added to every source file in the project, in fact it is not needed at all. However for some small percentage of people it will be helpful to have it in these files specifically, as they can make it easier to go through the wall of text.

EDIT: Also, this is not about forcing my vim configuration into these files so that it works just how I want it. This is about sharing this configuration so that others may benefit from it as well. Of course I could have this locally, but that's not the goal.

If this is not easily enabled in configuration file, then it's not a issue add it in source, anyway, it's better restrict this kind of configuration in source code, as there is too much IDE here and there

I did look up the meaning of the modeline, so it is clear what it is achieving. And I also agree that it may be worth adding such developer-assisting settings or configurations to the code base. Where I do not agree is where such settings should be placed. Single tool-specific (e.g., .clang-format) or non-code (e.g., .gitignore) files: yes. Public headers: less so. (BTW, actually even internal files use this doxygen syntax for grouping parts of large files (e.g., js-parser-internal.h or jmem-allocator-internal.h). So this is a project-wide setting, not only related to two public headers.)

You are correct that that internal sources also contain grouping directives, however that is also that would cause problems if we would set this project wide.

Source files usually only contain a single group for the entire file, and collapsing the whole source text isn't very helpful. Also in source files you'd normally prefer a different mode of folding, for example by functions or by blocks. So this setting only really makes sense for these headers, where a lot of groups are created.

One other difference is that tool specific config files are only really used by developers. This setting is not restricted to development purposes. For example, if one were to package the header, then someone reading it could make use of the modeline, whereas if it was set in a config file, that would most likely not be included.

docs/05.PORT-API.md

jerry-core/jrt/jrt.h

jerry-ext/include/jerryscript-ext/debugger.h

jerry-ext/util/repl.c

lygstate · 2022-01-13T15:02:03Z

tests/jerry/date-tostring.js

-assert (new Date ("2015-07-08T11:29:05.023").toTimeString () == "11:29:05 GMT+0000");
+assert (new Date ("2015-02-13").toGMTString () == "Fri, 13 Feb 2015 00:00:00 GMT");
+assert (new Date ("2015-07-08T11:29:05.023+01:00").toGMTString () == "Wed, 08 Jul 2015 10:29:05 GMT");
+assert (new Date ("2015-07-08T11:29:05.023+10:00").toGMTString () == "Wed, 08 Jul 2015 01:29:05 GMT");


Does this worth in seperate PR?. Seems not relate to jerry-port

It's somewhat related, as the prototype of the port api function for local tza has been changed, which required the implementation for it to be updated. This in turn fixed a former issue, which caused incorrect TZA handling in some cases.
These tests were testing the former incorrect behavior and needed to be updated.

lygstate · 2022-01-14T09:32:51Z

jerry-core/api/jerry-module.c

+jerry_module_resolve (const jerry_value_t specifier, /**< module specifier string */
+                      const jerry_value_t referrer, /**< parent module */
+                      void *user_p) /**< user data */
+{


What's the intension of user_p and not used by this function.

It is there to match the interface required by jerry_module_link.

Most of the time, jerry_module_resolve is not called directly, instead it is a callback in jerry_module_link. Embedders can also create custom module resolvers, and make jerry_module_link use those as callbacks.

The user_p is passed by jerry_module_link, which allows user data to be passed to the callback function in an arbitrary way. The default jerry_module_resolve does not require any extra user data, thats why it's unused, however another implementaion may do so.

lygstate · 2022-01-14T09:39:32Z

jerry-core/api/jerry-module.c

+
+  module_p->next_p = manager_p->module_head_p;
+  module_p->path_p = path_p;
+  module_p->basename_offset = jerry_port_path_base (module_p->path_p);


Maybe it's better add length parameter to jerry_port_path_base, there is no need strlen(module_p->path_p) again and again.

We don't know the length of the string where the function is called, and we don't necessarly want to either.
The port may not need the length at all, as it may just pass the string directly to a platform specific api, so calculating it in the core wouldn't be worth it. If the port implemntation needs the length, then it can calculate it as necessary.

lygstate · 2022-01-14T09:41:05Z

jerry-core/include/jerryscript-port.h

 */
-void jerry_port_print_char (char c);
+jerry_size_t jerry_port_path_base (const jerry_char_t *path_p);


Suggest add path_size parameter to jerry_port_path_base like jerry_port_path_normalize does

The doc states that param path_p must be zero terminated string.

The doc states that param path_p must be zero terminated string.

Still we can add the path_size paramter for consistence with jerry_port_path_normalize, and the first parameter of jerry_port_path_normalize also are zero terminated, the port user can choose what-ever they want, use zero terminating or string size directly.

See my comment here

lygstate · 2022-01-14T09:43:24Z

jerry-core/include/jerryscript-port.h

 */
-void jerry_port_module_release (const jerry_value_t realm);
+double jerry_port_current_time (void);


Suggest return int64_t instead of double, there is no need return double, as this is port

The retuned value is always used internally, so the double is more suitable.

According to https://tc39.es/proposal-temporal/docs/instant.html#epochNanoseconds

epochNanoseconds can be bigint, so maybe it's better to return nanoseconds instead,
as we using ms would lost precision.

Temporal.Instant does represent time in nanosecond precision, however there's no constructor for Temporal.Instant that uses the current system time, it can only be constructed by providing a user specified nanosecond value, or from other objects.

From what I take from the proposal, it's that Temporal.Instant is used to handle external time values with higher precision, for example from a database, and not for querying system time.

I don't think a lot of platforms support true nanosecond resolution either. Unix uses microseconds, I beleive Windows supports even less. Embedded platforms probably support less as well.

Therefore we don't really need to have a more precise port api than the default Date object can handle. Also, we could not return with a bigint from the port, that would add a dependency to the core.

Whether we should use double or int64 is I think debatable. On one hand ecmascript requires Date objects to use double precision format. This is very unlikely to change. On the other, platforms usually represent time in integral types. This is also something that will likely never change.

We will need to convert from integer to double somewhere, question is whether it would be beneficial to do it later, and allow the port to return "true" system time.

lygstate · 2022-01-14T09:47:34Z

jerry-core/include/jerryscript-port.h

+ *
+ * @return the pointer to the engine context.
+ */
+void jerry_port_context_free (void);


There is nothing returned,
I would suggest merge
jerry_port_context_free, jerry_port_line_free, jerry_port_path_free and jerry_port_source_free into single
jerry_port_free function.

And what if I have 4 different allocators? Somehow we must identify what we have to free. We could pass an extra argument but isn't it better to have an own free function for each operation. I think the answer is yes.

docs/05.PORT-API.md

jerry-core/include/jerryscript-core.h

rerobika · 2022-01-14T09:55:54Z

jerry-core/include/jerryscript-port.h

+ *
+ * @return the pointer to the engine context.
+ */
+void jerry_port_context_free (void);


And what if I have 4 different allocators? Somehow we must identify what we have to free. We could pass an extra argument but isn't it better to have an own free function for each operation. I think the answer is yes.

rerobika · 2022-01-14T09:58:09Z

jerry-core/include/jerryscript-port.h

 */
-void jerry_port_print_char (char c);
+jerry_size_t jerry_port_path_base (const jerry_char_t *path_p);


The doc states that param path_p must be zero terminated string.

jerry-port/common/jerry-port-fs.c

rerobika · 2022-01-14T10:01:38Z

jerry-core/include/jerryscript-port.h

 */
-void jerry_port_module_release (const jerry_value_t realm);
+double jerry_port_current_time (void);


The retuned value is always used internally, so the double is more suitable.

jerry-ext/util/repl.c

jerry-core/api/jerryscript.c

Notable changes: - Updated and the port API interface, new functions have been added and some have been changed. The port library is now cleaned up to not have any dependency on jerry-core, as it should be. The port library is now strictly a collection of functions that implement embedding/platform specific behavior. - The default port implementation has been split for windows and unix. Implemented port functions have been categorized and reorganized, and marked with attribute((weak)) for better reusability. - External context allocation has been moved to the port API instead of a core API callback. The iterface has also been extended with a function to free the allocated context. When external context is enabled, jerry_init now automatically calls the port implementation to allocate the context and jerry_cleanup automatically calls the port to free the context. - jerry_port_log has been changed to no longer require formatting to be implemented by the port. The reason beind this is that it was vague what format specifiers were used by the engine, and in what manner. The port function now takes a zero-terminated string, and should only implement how the string should be logged. - Logging and log message formatting is now handled by the core jerry library where it can be implemented as necessary. Logging can be done through a new core API function, which uses the port to output the final log message. - Log level has been moved into jerry-core, and an API function has been added to set the log level. It should be the library that filters log messages based on the requested log level, instead of logging everything and requiring the user to do so. - Module resolving logic has been moved into jerry-core. There's no reason to have it in the port library and requiring embedders to duplicate the code. It also added an unnecessary dependency on jerry-core to the port. Platform specific behavior is still used through the port API, like resolving module specifiers, and reading source file contents. If necessary, the resolving logic can still be overridden as previously. - The jerry-ext library has also been cleaned up, and many utility functions have been added that previously were implemented in jerry-main. This allows easier reusability for some common operations, like printing unhandled exceptions or providing a repl console. - Debugger interaction with logged/printed messages has been fixed, so that it's no longer the port implementations responsibility to send the output to the debugger, as the port should have no notion of what a debugger is. The printing and logging functions will now pass the result message to the debugger, if connected. - Cleaned up TZA handling in the date port implementation, and simplified the API function prototype. - Moved property access helper functions that use ASCII strings as keys from jerry-ext to the core API. JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]

galpeter

lgtm

PR jerryscript-project#4907 moved the log level into the engine context, however this caused issues with logging without the engine being initialized. This commit reverts the log level to be a static variable. This commit also implements missing format specifiers for jerry_log. JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]

PR #4907 moved the log level into the engine context, however this caused issues with logging without the engine being initialized. This commit reverts the log level to be a static variable. This commit also implements missing format specifiers for jerry_log. JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]

dbatyai added api Related to the public API jerry-port Related to the port API or the default port implementation jerry-ext Related to the jerry-ext library labels Dec 15, 2021

dbatyai force-pushed the port-api-update branch 7 times, most recently from a396a3a to ca4ae44 Compare December 15, 2021 16:58

dbatyai force-pushed the port-api-update branch from ca4ae44 to e72abcf Compare January 10, 2022 13:18

dbatyai force-pushed the port-api-update branch 7 times, most recently from d0c9d6c to 4b53baf Compare January 12, 2022 13:04

lygstate reviewed Jan 13, 2022

View reviewed changes

jerry-port/win/jerry-port-win-fs.c Outdated Show resolved Hide resolved

lygstate reviewed Jan 13, 2022

View reviewed changes

galpeter reviewed Jan 13, 2022

View reviewed changes

docs/05.PORT-API.md Outdated Show resolved Hide resolved

jerry-core/jrt/jrt.h Outdated Show resolved Hide resolved

jerry-ext/include/jerryscript-ext/debugger.h Outdated Show resolved Hide resolved

jerry-ext/util/repl.c Outdated Show resolved Hide resolved

lygstate reviewed Jan 13, 2022

View reviewed changes

dbatyai force-pushed the port-api-update branch 3 times, most recently from 02b90aa to 0f7d78c Compare January 13, 2022 22:51

lygstate reviewed Jan 14, 2022

View reviewed changes

rerobika reviewed Jan 14, 2022

View reviewed changes

dbatyai force-pushed the port-api-update branch from 0f7d78c to fe6dbb3 Compare January 14, 2022 12:12

rerobika approved these changes Jan 17, 2022

View reviewed changes

jerry-core/api/jerryscript.c Outdated Show resolved Hide resolved

dbatyai force-pushed the port-api-update branch 5 times, most recently from 3593b39 to ecb47b3 Compare January 19, 2022 11:50

galpeter approved these changes Jan 20, 2022

View reviewed changes

dbatyai merged commit ac1c48e into jerryscript-project:master Jan 20, 2022

dbatyai mentioned this pull request Jan 21, 2022

Fix logging related issues #4971

Merged

dbatyai mentioned this pull request Feb 9, 2022

temporal.now require nanosecond accuracy #4975

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update jerry-port and jerry-ext #4907

Update jerry-port and jerry-ext #4907

dbatyai commented Dec 15, 2021 •

edited

Loading

lygstate commented Dec 26, 2021

dbatyai commented Jan 10, 2022

lygstate Jan 13, 2022

dbatyai Jan 13, 2022

lygstate Jan 13, 2022

akosthekiss Jan 13, 2022

dbatyai Jan 13, 2022 •

edited

Loading

akosthekiss Jan 14, 2022

dbatyai Jan 14, 2022 •

edited

Loading

lygstate Jan 14, 2022

akosthekiss Jan 17, 2022

dbatyai Jan 17, 2022

lygstate Jan 13, 2022

dbatyai Jan 13, 2022

lygstate Jan 14, 2022

dbatyai Jan 14, 2022 •

edited

Loading

lygstate Jan 14, 2022

dbatyai Jan 14, 2022

lygstate Jan 14, 2022 •

edited

Loading

rerobika Jan 14, 2022

lygstate Jan 14, 2022

dbatyai Jan 14, 2022

lygstate Jan 14, 2022

rerobika Jan 14, 2022

lygstate Jan 14, 2022

dbatyai Jan 14, 2022 •

edited

Loading

lygstate Jan 14, 2022

rerobika Jan 14, 2022

rerobika Jan 14, 2022

rerobika Jan 14, 2022

rerobika Jan 14, 2022

galpeter left a comment

Update jerry-port and jerry-ext #4907

Update jerry-port and jerry-ext #4907

Conversation

dbatyai commented Dec 15, 2021 • edited Loading

lygstate commented Dec 26, 2021

dbatyai commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatyai Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatyai Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatyai Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lygstate Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatyai Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galpeter left a comment

Choose a reason for hiding this comment

dbatyai commented Dec 15, 2021 •

edited

Loading

dbatyai Jan 13, 2022 •

edited

Loading

dbatyai Jan 14, 2022 •

edited

Loading

dbatyai Jan 14, 2022 •

edited

Loading

lygstate Jan 14, 2022 •

edited

Loading

dbatyai Jan 14, 2022 •

edited

Loading