Skip to content

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

Merged

Conversation

LaszloLango
Copy link
Contributor

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

@LaszloLango LaszloLango added api Related to the public API documentation Related to documentation labels Aug 22, 2018
@@ -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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.


**Example**

```c
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

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 */
};
Copy link
Member

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.

@LaszloLango
Copy link
Contributor Author

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

Copy link
Member

@akosthekiss akosthekiss left a 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.

#include "jerryscript-port.h"

/* A different Thread Local Storage variable for each jerry instance. */
__thread jerry_instance_t* tls_instance;
Copy link
Member

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

instance_alloc_fn (size_t size, void *cb_data)
{
(void) cb_data;
return malloc(size);
Copy link
Member

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 (

Copy link
Member

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?

pthread_t threads[NUM_OF_THREADS];

/* Create the threads. */
for (int i = 0; i < NUM_OF_THREADS; i++) {
Copy link
Member

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

}

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

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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(added below)

…ence.

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

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@akosthekiss akosthekiss merged commit df18930 into jerryscript-project:master Aug 30, 2018
@LaszloLango LaszloLango deleted the context-doc-update branch September 20, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the public API documentation Related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants