Fix FILE_PATTERNS of Doxyfile and some of the issues it was hiding #2446

akosthekiss · 2018-08-06T10:21:36Z

FILE_PATTERNS is a space-separated list, in contrary to what is
suggested by its documentation. The pattern *.h, *.c does not
match header files but files with .h, extension only. Rewriting
the current comma-separated pattern makes Doxygen actually process
header files. However, it also reveals several hitherto hidden
issues (mostly missing documentation) in the code. This patch fixes
some of these documentation problems (and lists the files with
outstanding issues in a 'backlog').

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

akosthekiss · 2018-08-06T10:27:36Z

Note: Doxygen has not been processing headers since #1540 .

Note 2: The list of files with still missing documentation (i.e., not fixed by this PR) is:

jerry-core/ecma/base/ecma-globals.h
jerry-core/ecma/base/ecma-helpers.h
jerry-core/ecma/operations/ecma-exceptions.h
jerry-core/include/jerryscript-debugger-transport.h
jerry-core/jcontext/jcontext.h
jerry-core/parser/js/byte-code.h
jerry-core/parser/js/common.h
jerry-core/parser/js/js-lexer.h
jerry-core/parser/js/js-parser-internal.h
jerry-core/parser/regexp/re-parser.h
jerry-core/vm/vm-stack.h

I'm happy to break them out into an issue to track the progress of their fixes, should the PR be accepted and the issue considered valid. (But I cannot volunteer to document them.)

zherczeg · 2018-08-06T10:32:42Z

jerry-core/ecma/base/ecma-globals.h

     * Description of pseudo array objects.
     */
    struct
    {
      uint8_t type; /**< pseudo array type, e.g. Arguments, TypedArray*/
-      uint8_t extra_info; /**< extra infomations about the object.
-                            *  e.g. element_width_shift for typed arrays */
+      uint8_t extra_info; /**< extra infomation about the object.


infromation

information

zherczeg · 2018-08-06T10:34:19Z

jerry-core/ecma/base/ecma-globals.h

- * Minimum positive and maximum value of ecma-number
+ * Number.MIN_VALUE (i.e., the smallest positive value of ecma-number)
+ *
+ * See also: ECMA_262 v5, 15.7.3.3


I don't really like these comment duplications. Is there a better way to do this?

There are no easy ways and/or this is the way that's compatible with the rest of ecma-globals.h. (E.g., see the documentation of ecma_number_t, ECMA_NUMBER_MAX_DIGITS, ECMA_NUMBER_SIGN_WIDTH, ECMA_NUMBER_BIASED_EXP_WIDTH, and ECMA_NUMBER_FRACTION_WIDTH.) I've followed that approach here, too.

FYI, there may be one way to reduce duplication by using @def MACRONAME(args) doxygen syntax to break out documentation from before the macro definitions, but that might necessitate a bigger rewrite throughout the code base (to apply that approach everywhere where macros (or typedefs, or functions) are conditionally defined multiple times). Moreover, that approach should be discussed further as it splits the documentation from the documented entity, making maintenance somewhat harder. So, I'd have that separate from this PR.

(To restate: this PR tries to fix the problem in the Doxyfile with the least friction, fixing only the fallout that can be easily fixed, leaving the rest for follow-up.)

You can use @copydoc or groups to avoid duplication in comments. Maybe my previous PR can help: #2324

But you cannot copy documentation when an entity is defined multiple times because of conditional compilation, can you? I guess you cannot copydoc self.

#if COND /** documentation of X */ #define X /** documentation of Y */ #define Y #else /** @copydoc X */ // <-- THIS SHOULDN'T WORK #define X /** @copydoc Y */ // <-- THIS SHOULDN'T WORK #define Y #endif

IHMO the default macro conditions should be documented. The Doxygen docs will be generated with the default config. BTW you can use a group comment before the #if which should work independently from the configuration. Although this can be useful in simple cases only (like your example in the prev. comment).

zherczeg · 2018-08-06T10:35:27Z

jerry-core/include/jerryscript-compiler.h

@@ -73,61 +73,111 @@ void * __cdecl _alloca (size_t _Size);
 /*
 * Attributes
 */
+
+/**
+ * Function attribute to direct compiler to align function to given number of


I would remove these "to direct compiler" parts.

zherczeg · 2018-08-06T10:36:44Z

jerry-core/parser/js/common.h

@@ -57,21 +57,21 @@ typedef enum

 /* Flags for status_flags. */

-/* Local identifier (var, function arg). */
+/** Local identifier (var, function arg). */


Hm, would be better to convert these into an enum.

I won't be pursuing that in this PR, which is solely about documentation fixes.

zherczeg · 2018-08-06T10:37:16Z

jerry-core/parser/js/js-parser.h

@@ -122,7 +122,7 @@ typedef enum
  PARSER_ERR_NON_STRICT_ARG_DEFINITION                /**< non-strict argument definition */
 } parser_error_t;

-/* Source code line counter type. */
+/** Source code line counter type. */


We should use the multiline form here.

akosthekiss · 2018-08-06T15:38:49Z

PR updated, actioned most of the review comments

akosthekiss · 2018-08-08T21:21:41Z

PR rebased to latest master

zherczeg · 2018-08-09T06:40:16Z

jerry-core/include/jerryscript-compiler.h

+/**
+ * Function attribute to trigger warning if function's caller doesn't use (e.g.,
+ * check) the return value.
+ */
 #ifndef JERRY_ATTR_WARN_UNUSED_RESULT
 #define JERRY_ATTR_WARN_UNUSED_RESULT
 #endif /* !JERRY_ATTR_WARN_UNUSED_RESULT */

 /*
 * Condition likeliness, unlikeliness
 */


Shall we keep this comment? It seems unnecessary now.

It is/was only a grouping comment by I can remove it. But then I'll remove the other grouping comment ("Attributes"), too.

zherczeg · 2018-08-09T06:41:00Z

jerry-core/ecma/base/ecma-globals.h

@@ -142,7 +142,7 @@ typedef int32_t ecma_integer_value_t;
 */
 #define ECMA_DIRECT_SHIFT 4

-/* ECMA make simple value */
+/** ECMA make simple value */


I would turn this a 3 line form.

`FILE_PATTERNS` is a space-separated list, in contrary to what is suggested by its documentation. The pattern `*.h, *.c` does not match header files but files with `.h,` extension only. Rewriting the current comma-separated pattern makes Doxygen actually process header files. However, it also reveals several hitherto hidden issues (mostly missing documentation) in the code. This patch fixes some of these documentation problems (and lists the files with outstanding issues in a 'backlog'). JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

akosthekiss · 2018-08-09T08:12:19Z

PR updated

zherczeg

LGTM

yichoi

LGTM

akosthekiss added enhancement An improvement documentation Related to documentation labels Aug 6, 2018

zherczeg requested changes Aug 6, 2018

View reviewed changes

akosthekiss force-pushed the doxygen-fixes branch from c819d28 to 1f08cd4 Compare August 6, 2018 15:36

akosthekiss force-pushed the doxygen-fixes branch from 1f08cd4 to 4f14877 Compare August 8, 2018 20:08

zherczeg reviewed Aug 9, 2018

View reviewed changes

akosthekiss force-pushed the doxygen-fixes branch from 4f14877 to 236d37c Compare August 9, 2018 08:11

zherczeg approved these changes Aug 9, 2018

View reviewed changes

yichoi approved these changes Aug 13, 2018

View reviewed changes

yichoi merged commit 79d1a3c into jerryscript-project:master Aug 13, 2018

akosthekiss deleted the doxygen-fixes branch August 13, 2018 07:59

akosthekiss mentioned this pull request Aug 13, 2018

Missing documentation from jerry-core headers #2469

Open

11 tasks

Fix FILE_PATTERNS of Doxyfile and some of the issues it was hiding #2446

Fix FILE_PATTERNS of Doxyfile and some of the issues it was hiding #2446

Uh oh!

Conversation

akosthekiss commented Aug 6, 2018

Uh oh!

akosthekiss commented Aug 6, 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!

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 commented Aug 6, 2018

Uh oh!

akosthekiss commented Aug 8, 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!

akosthekiss commented Aug 9, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

yichoi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!