Move version numbers to public headers #2556

Achie72 · 2018-10-08T08:10:09Z

JERRY_SNAPSHOT_VERSION and JERRY_DEBUGGER_VERSION were moved into public headers, to grant access to them.

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

zherczeg · 2018-10-08T11:40:57Z

docs/02.API-REFERENCE.md

+
+Enum that contains the following elements:
+
+  - JERRY_VERSION_API_MAJOR - signals the major API version


comment: returns the...
Please also modify the others.

zherczeg · 2018-10-08T11:41:59Z

docs/02.API-REFERENCE.md

+
+```c
+uint32_t
+jerry_get_api_major_version(void);


These functions are redundant, aren't they?

galpeter · 2018-10-08T11:34:23Z

docs/02.API-REFERENCE.md

+
+Enum that contains the following elements:
+
+  - JERRY_VERSION_API_MAJOR - signals the major API version


What if we would change the "signals" word, to "represents" / "describes"?

galpeter · 2018-10-08T11:35:40Z

docs/02.API-REFERENCE.md

+  - JERRY_VERSION_API_MAJOR - signals the major API version
+  - JERRY_VERSION_API_MINOR - signals the minor API version
+  - JERRY_VERSION_DEBUGGER - signals the debugger version
+  - JERRY_VERSION_SNAPSHOT - signals the snapshot version


As these values are only used for one method we should add a "see also" block to refer the method which uses this.

galpeter · 2018-10-08T11:37:29Z

docs/02.API-REFERENCE.md

+- return value
+  - API minor version
+
+## jerry_get_component_version


If we have this method, why do we need the previous two? (get_api_minor & get_api_major)?

galpeter · 2018-10-08T11:38:23Z

docs/02.API-REFERENCE.md

+**Summary**
+
+Get the component version described in parameter. If the defined components
+asked is not available, then it returns 0, as well as a default value.


What do you mean by: ..it returns 0, as well as a default value. ? This will return a zero and another value?

galpeter · 2018-10-08T11:39:15Z

docs/02.API-REFERENCE.md

+```
+- `component` - A jerry_versions_t signaling desired component.
+- return value
+  - API major version


AFAIK: this is a typo here: the return value is the version of the requested component.

galpeter · 2018-10-08T11:42:29Z

docs/02.API-REFERENCE.md

+
+```c
+uint32_t api_version;
+api_version = jerry_get_component_version(API_MAJOR_VERSION);


There is no API_MAJOR_VERSION described in the jerry_version_t enum.

galpeter · 2018-10-08T11:42:52Z

docs/02.API-REFERENCE.md

@@ -5558,6 +5623,7 @@ jerry_get_arraybuffer_byte_length (const jerry_value_t value);
 ```

 **See also**
+


Is this empty line needed here?

galpeter · 2018-10-08T11:44:47Z

jerry-core/api/jerry.c

+ * Get the component version described in parameter.
+ *
+ * Note:
+ *    * Debugger and snapshot versions are set to undefined, unless they are turned on.


The comment states that the "debugger/snapshot version are set to undefined" if they are turned off. But in the code the return value will be 0. That looks pretty defined for me....

galpeter · 2018-10-08T11:45:14Z

jerry-core/api/jerry.c

+ *    * Debugger and snapshot versions are set to undefined, unless they are turned on.
+ *
+ * @return a uint32_t containing:
+ *         API major, API minor, debugger and snapshot version.


So.. this will return all of these values? in one uint32_t?

galpeter · 2018-10-08T11:45:54Z

jerry-core/api/jerry.c

+ *         API major, API minor, debugger and snapshot version.
+ */
+uint32_t
+jerry_get_component_version (jerry_version_t component)  /**< the component, the version of which we want */


This comment reads a bit awkward.

akosthekiss

Please, convince me that we need functions to achieve this. include/jerryscript-core.h already has JERRY_API_MAJOR_VERSION and JERRY_API_MINOR_VERSION as macro-defined constants. api/jerry-snapshot.h already has JERRY_SNAPSHOT_VERSION, which could be moved to include/jerryscript-snapshot.h. And debugger/debugger.h already has JERRY_DEBUGGER_VERSION, which could be moved to include/jerryscript-debugger.h. I see no point in hiding these behind functions nor introducing a new enum to programmatically choose from different version numbers. All that's needed is to publish two already existing macros in public headers. IMO.

Achie72 · 2018-10-11T09:52:32Z

It could help, if we only have an binary file, to confirm, which version of jerry is it exactly, to avoid confusion.

akosthekiss · 2018-10-11T11:22:20Z

I don't really understand this use case.

If "binary file" means a library (most probably, a static library) then it MUST be accompanied by the appropriate public header files to be useful. But then the macros in the public header file(s) specify the versions. (Using a libxxx.a without an xxx.h is certainly possible at the technical level but that's called reverse engineering and is not a use case library developers (== us) have to support.)

If "binary file" means an executable application then it's the task of the application to identify itself or the components it contains. In this case, the application source code can contain JERRY_API_MAJOR_VERSION just as easily as jerry_get_component_version (...). If the app source contains any of these then the version can be confirmed (e.g., printed by app --version), if not, then functions don't help either.

zherczeg · 2018-10-11T11:44:45Z

He is probably talking about dynamic modules, where you load a module and try to confirm the module is what you expected.

akosthekiss · 2018-10-11T12:16:16Z

Dynamic libraries are also a tricky thing. If one knows the API version of the library (the major version at least) then one may know what functions can be expected to exist (at least the minimum subset). But that means that one already has to know the value of JERRY_API_MAJOR_VERSION even to call jerry_get_component_version (JERRY_VERSION_API_MAJOR) (because only at a given major version would it be guaranteed to exist). Almost a deadlock.

Moreover, although using jerry-core as a dynamic library is certainly possible, the device class where that's possible is pretty much not the main application area of the engine.

Achie72 · 2018-10-12T06:57:02Z

The moving of JERRY_SNAPSHOT_VERSION and JERRY_DEBUGGER_VERSION indeed seems a more simple solution, than the getter functions. If we don't want to complicate the code by introducing these changes, then i can modify the PR with your suggestion, if the other reviewers have no problem with it. @zherczeg @galpeter ?

zherczeg · 2018-10-12T07:17:05Z

I have no strong opinion about this, so whatever is good for me.

JERRY_SNAPSHOT_VERSION and JERRY_DEBUGGER_VERSION were moved into public headers, to grant access to them. JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

akosthekiss

LGTM

zherczeg

LGTM

Achie72 force-pushed the queryInfo branch from df14eb5 to 89a8a04 Compare October 8, 2018 08:46

zherczeg requested changes Oct 8, 2018

View reviewed changes

galpeter reviewed Oct 8, 2018

View reviewed changes

akosthekiss requested changes Oct 8, 2018

View reviewed changes

Move version numbers to public headers

2ee04d0

JERRY_SNAPSHOT_VERSION and JERRY_DEBUGGER_VERSION were moved into public headers, to grant access to them. JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

Achie72 force-pushed the queryInfo branch from 89a8a04 to 2ee04d0 Compare October 15, 2018 08:31

Achie72 changed the title ~~Add API function to retrieve API, debugger, and snapshot version~~ Move version numbers to public headers Oct 15, 2018

akosthekiss approved these changes Oct 17, 2018

View reviewed changes

akosthekiss added api Related to the public API snapshot Related to the snapshot feature debugger Related to the debugger labels Oct 17, 2018

zherczeg approved these changes Oct 19, 2018

View reviewed changes

zherczeg merged commit f8f691d into jerryscript-project:master Oct 19, 2018

akosthekiss mentioned this pull request Jan 10, 2019

Export macro JERRY_SNAPSHOT_VERSION #1751

Closed


		Enum that contains the following elements:

		- JERRY_VERSION_API_MAJOR - signals the major API version

		@@ -5558,6 +5623,7 @@ jerry_get_arraybuffer_byte_length (const jerry_value_t value);
		```

		See also

Move version numbers to public headers #2556

Move version numbers to public headers #2556

Uh oh!

Conversation

Achie72 commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Achie72 commented Oct 11, 2018

Uh oh!

akosthekiss commented Oct 11, 2018

Uh oh!

zherczeg commented Oct 11, 2018

Uh oh!

akosthekiss commented Oct 11, 2018

Uh oh!

Achie72 commented Oct 12, 2018

Uh oh!

zherczeg commented Oct 12, 2018

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Achie72 commented Oct 8, 2018 •

edited

Loading