-
Notifications
You must be signed in to change notification settings - Fork 684
Added missing documentation of JerryScript instances to the API reference #2482
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
Added missing documentation of JerryScript instances to the API reference #2482
Conversation
@@ -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. |
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.
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.
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 it is needed to make it consistent.
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.
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.
docs/02.API-REFERENCE.md
Outdated
|
||
**Example** | ||
|
||
```c |
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.
Would it be possible to make this example doctested?
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 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.
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.
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.
9f67f46
to
69cf785
Compare
docs/02.API-REFERENCE.md
Outdated
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 */ | ||
}; |
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 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.
69cf785
to
5e0bfd2
Compare
@akosthekiss I've updated the PR. Please check. |
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've spotted some deviations from the JerryScript coding style guidelines. Those should be followed in doc examples, too.
- 2 extra remarks.
docs/02.API-REFERENCE.md
Outdated
#include "jerryscript-port.h" | ||
|
||
/* A different Thread Local Storage variable for each jerry instance. */ | ||
__thread jerry_instance_t* tls_instance; |
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.
the asterisk seems to stick to the wrong side
docs/02.API-REFERENCE.md
Outdated
instance_alloc_fn (size_t size, void *cb_data) | ||
{ | ||
(void) cb_data; | ||
return malloc(size); |
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.
space is missing from between malloc and (
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.
and one more question: where will the here-allocated memory be freed?
docs/02.API-REFERENCE.md
Outdated
pthread_t threads[NUM_OF_THREADS]; | ||
|
||
/* Create the threads. */ | ||
for (int i = 0; i < NUM_OF_THREADS; i++) { |
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.
open brace should be on its own line
docs/02.API-REFERENCE.md
Outdated
} | ||
|
||
/* Wait for the threads to complete, and release their resources. */ | ||
for (int i = 0; i < NUM_OF_THREADS; i++) { |
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.
ditto
**See also** | ||
|
||
- [jerry_instance_t](#jerry_instance_t) | ||
- [jerry_instance_alloc_t](#jerry_instance_alloc_t) |
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 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.
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 was wondering on this too, but could not find a good solution which works both with MD and the generated jerryscript.net
page.
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.
(added below)
5e0bfd2
to
b3c93f8
Compare
…ence. JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]
b3c93f8
to
bd4a0de
Compare
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.
LGTM
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.
LGTM
JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]