Added missing documentation of JerryScript instances to the API reference #2482

LaszloLango · 2018-08-22T08:54:57Z

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

akosthekiss · 2018-08-22T10:01:54Z

jerry-core/include/jerryscript-core.h

@@ -529,7 +529,7 @@ jerry_value_t jerry_resolve_or_reject_promise (jerry_value_t promise, jerry_valu
 bool jerry_is_valid_utf8_string (const jerry_char_t *utf8_buf_p, jerry_size_t buf_size);
 bool jerry_is_valid_cesu8_string (const jerry_char_t *cesu8_buf_p, jerry_size_t buf_size);

-/*
+/**
 * External context functions.


Is this change needed? Isn't this function doxygen-documented in the sources? Also, plural (functionS) is strange here as this is a single function only. And this comment is not the same as in the .md file. If this function is to be documented here (and not in the sources) then please copy here the one line description from the .md summary.

PS: The doxygen doc comments below are also strange, as they seem to mark up a group of functions but I doubt that doxygen will add that documentation to all of them.

IMHO it is needed to make it consistent.

Well, if we want to be consistent and correct, we should remove the extra asterisks from everywhere in this header, IMO. /** is for doctesting one entity, not for marking up groups.

akosthekiss · 2018-08-22T10:22:00Z

docs/02.API-REFERENCE.md

+
+**Example**
+
+```c


Would it be possible to make this example doctested?

I wanted to do, but I ran into linking problems. I did not want to add -pthread to all doctest build. AFAIK doctest comments does not support additional build options.

That's correct, linking wont work. But you can instruct doctest to compile only (there are examples for that in the existing documentation). Compilation can already catch lots of potential errors.

akosthekiss · 2018-08-27T22:18:09Z

docs/02.API-REFERENCE.md

+  jmem_heap_t *heap_p; /**< point to the heap aligned to JMEM_ALIGNMENT (is not used with system allocator) */
+  uint32_t heap_size; /**< size of the heap (is not used with system allocator) */
+  uint8_t *lcache_p; /**< point to the entrance of the lcache in buffer */
+};


I don't think that jerry_instance_t should be documented in the API reference. That type is opaque from the perspective of the API, i.e., only pointers to the type are passed across the API boundary and the API user is never assumed to know how the struct really looks like, what size it is, etc. jerryscript-core.h documents it as "A forward declaration of the JerryScript instance structure", which should be enough to mention here, too. Perhaps replacing "forward" with "opaque" both here and in the header might be even better.

LaszloLango · 2018-08-29T06:52:42Z

@akosthekiss I've updated the PR. Please check.

akosthekiss

I've spotted some deviations from the JerryScript coding style guidelines. Those should be followed in doc examples, too.

2 extra remarks.

akosthekiss · 2018-08-29T07:35:17Z

docs/02.API-REFERENCE.md

+#include "jerryscript-port.h"
+
+/* A different Thread Local Storage variable for each jerry instance. */
+__thread jerry_instance_t* tls_instance;


the asterisk seems to stick to the wrong side

akosthekiss · 2018-08-29T07:36:30Z

docs/02.API-REFERENCE.md

+instance_alloc_fn (size_t size, void *cb_data)
+{
+  (void) cb_data;
+  return malloc(size);


space is missing from between malloc and (

and one more question: where will the here-allocated memory be freed?

akosthekiss · 2018-08-29T07:37:08Z

docs/02.API-REFERENCE.md

+  pthread_t threads[NUM_OF_THREADS];
+
+  /* Create the threads. */
+  for (int i = 0; i < NUM_OF_THREADS; i++) {


open brace should be on its own line

akosthekiss · 2018-08-29T07:37:23Z

docs/02.API-REFERENCE.md

+  }
+
+  /* Wait for the threads to complete, and release their resources. */
+  for (int i = 0; i < NUM_OF_THREADS; i++) {


akosthekiss · 2018-08-29T07:45:30Z

docs/02.API-REFERENCE.md

+**See also**
+
+- [jerry_instance_t](#jerry_instance_t)
+- [jerry_instance_alloc_t](#jerry_instance_alloc_t)


I don't think we have an example for cross referencing doc MD pages, but somehow the jerry_port_get_instance function should be mentioned here (even if without a link). Right now no text mentions it, it only appears in the example out of the blue, without any hints why it is needed.

I was wondering on this too, but could not find a good solution which works both with MD and the generated jerryscript.net page.

(added below)

…ence. JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

akosthekiss

LGTM

zherczeg

LGTM

LaszloLango added api Related to the public API documentation Related to documentation labels Aug 22, 2018

akosthekiss reviewed Aug 22, 2018

View reviewed changes

LaszloLango force-pushed the context-doc-update branch from 9f67f46 to 69cf785 Compare August 22, 2018 11:36

akosthekiss requested changes Aug 27, 2018

View reviewed changes

LaszloLango force-pushed the context-doc-update branch from 69cf785 to 5e0bfd2 Compare August 29, 2018 06:51

akosthekiss requested changes Aug 29, 2018

View reviewed changes

LaszloLango force-pushed the context-doc-update branch from 5e0bfd2 to b3c93f8 Compare August 29, 2018 08:22

Added missing documentation of JerryScript instances to the API refer…

bd4a0de

…ence. JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

LaszloLango force-pushed the context-doc-update branch from b3c93f8 to bd4a0de Compare August 29, 2018 10:45

akosthekiss approved these changes Aug 29, 2018

View reviewed changes

zherczeg approved these changes Aug 30, 2018

View reviewed changes

akosthekiss merged commit df18930 into jerryscript-project:master Aug 30, 2018

LaszloLango deleted the context-doc-update branch September 20, 2018 14:56


		Example

		```c

Added missing documentation of JerryScript instances to the API reference #2482

Added missing documentation of JerryScript instances to the API reference #2482

Uh oh!

Conversation

LaszloLango commented Aug 22, 2018

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!

LaszloLango commented Aug 29, 2018

Uh oh!

akosthekiss left a comment

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!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!