Harmony
  1. Harmony
  2. HARMONY-3284

[drlvm][doc] scarce comments in OS portability layer external interface headers

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None
    • Estimated Complexity:
      Advanced

      Description

      Many files lack ample and well-formatted comments; need to get easily readable, complete and useful reference for DRLVM Interface Reference and Java class library reference; the code commenting BKMs (http://wiki.apache.org/harmony/Code_Commenting) can be useful. Specific suggestions on improving Doxygen output are below.

      1. 2include_headers.patch
        15 kB
        Svetlana Konovalova

        Issue Links

          Activity

          Hide
          Svetlana Konovalova added a comment -

          Here are suggestions on how to improve code comments of the Thread manager component so that Doxygen parses them correctly.

          GENERAL NOTE: I do respect the authors of all these headers and I do understand how proud they are to see their names in the comments! But, since it's the open source, I'd like to ask you to remove the authors' names from the files content. Thanks in advance and sorry about that!

          OS portability layer
          APR extension

          port/include/apr_thread_ext.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions

          port/include/clog.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions

          port/include/cxxlog.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/lil.h
          -Add detailed description
          -Add brief description [@file]

          • Use appropriate formatting to make comments VISIBLE
          • Add missing descriptions
          • Use \ingroup, \defgroup (if applicable)
            -Define parameters as [in]/[out]
            -Use @return & @note
          • Use <code> tags for code entities
          • Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers.

          port/include/lil_code_generator.h
          -Add detailed description
          -Add brief description [@file]

          • Use appropriate formatting to make comments VISIBLE

          port/include/lil_code_generator_utils.h
          -Add detailed description
          -Add brief description [@file]

          • Use appropriate formatting to make comments VISIBLE
          • Add missing descriptions

          port/include/log_macro.h
          -Add detailed description

          • Add one missing description

          port/include/logger.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use \ingroup, \defgroup (if applicable)

          port/include/loggerstring.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions
          • Use \ingroup, \defgroup (if applicable)

          port/include/logparams.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions

          port/include/m2n.h
          -Add detailed description
          -Add brief description [@file]

          • Use appropriate formatting to make comments VISIBLE
          • Add missing descriptions
          • Use <code> tags for code entities

          port/include/platform_core_natives.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions

          port/include/port_atomic.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
            -Use @return & @note

          port/include/port_disasm.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Define parameters as <in> / <out>

          port/include/port_dso.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers.

          port/include/port_filepath.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use \ingroup, \defgroup (if applicable)

          port/include/port_general.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/port_malloc.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions
          • Use appropriate formatting

          port/include/port_sysinfo.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/port_timer.h
          -Add detailed description
          -Add brief description [@file]

          port/include/port_vmem.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers.
            -Define parameters as [in]/[out]

          port/include/stack_iterator.h
          -Use <code> tags for code entities

          port/include/tl/allocator.h
          -Add detailed description
          -Add brief description [@file]

          • Use appropriate formatting to make comments VISIBLE

          port/include/tl/list_mt.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/tl/memory_pool.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/tl/set_mt.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use appropriate formatting

          port/include/tl/set_mt.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/tl/vector.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/include/tl/vector_mt.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions

          port/src/encoder/dec_base.h

          • Add missing descriptions
          • Use appropriate formatting to split code and comments

          port/src/encoder/enc_base.h

          • Add missing descriptions
          • Use <code> tags for code entities
          • Define parameters as <in> / <out>
            -Add detailed description
          • No need to put @brief in the beginning of function descriptions. Please remove.

          port/src/encoder/enc_defs.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Use appropriate formatting
            -Use @return & @note

          port/src/encoder/enc_prvt.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Use appropriate formatting

          port/src/encoder/encoder.h

          • Add missing descriptions
          • Use appropriate formatting
          • Use \ingroup, \defgroup (if applicable)

          port/src/lil/em64t/pim/include/lil_code_generator_em64t.h
          -Add detailed description
          -Add brief description [@file]

          • begin sentences with the capital letter.
          • Use <code> tags for code entities

          port/src/lil/ia32/pim/include/lil_code_generator_ia32.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions

          port/src/lil/ipf/pim/include/lil_code_generator_ipf.h
          -Add detailed description
          -Add brief description [@file]

          • Document functions
          • Use appropriate formatting

          port/src/lil/em64t/pim/m2n_em64t_internal.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers.
            -Define parameters as [in]/[out]

          port/src/lil/ia32/pim/m2n_ia32_internal.h
          -Use appropriate formatting
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities

          port/src/lil/ipf/pim/m2n_ipf_internal.h
          -Add detailed description
          -Add brief description [@file]

          • Add missing descriptions
          • Use <code> tags for code entities
          • Use appropriate formatting to make comments VISIBLE

          Component manager

          include/open/compmgr.h
          -Add detailed description (if applicable)

          • Use <code> tags for code entities

          include/component_manager.h
          -Add detailed description
          -Add brief description [@file]

          Show
          Svetlana Konovalova added a comment - Here are suggestions on how to improve code comments of the Thread manager component so that Doxygen parses them correctly. GENERAL NOTE: I do respect the authors of all these headers and I do understand how proud they are to see their names in the comments! But, since it's the open source, I'd like to ask you to remove the authors' names from the files content. Thanks in advance and sorry about that! OS portability layer APR extension port/include/apr_thread_ext.h -Add detailed description -Add brief description [@file] Document functions port/include/clog.h -Add detailed description -Add brief description [@file] Document functions port/include/cxxlog.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/lil.h -Add detailed description -Add brief description [@file] Use appropriate formatting to make comments VISIBLE Add missing descriptions Use \ingroup, \defgroup (if applicable) -Define parameters as [in] / [out] -Use @return & @note Use <code> tags for code entities Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers. port/include/lil_code_generator.h -Add detailed description -Add brief description [@file] Use appropriate formatting to make comments VISIBLE port/include/lil_code_generator_utils.h -Add detailed description -Add brief description [@file] Use appropriate formatting to make comments VISIBLE Add missing descriptions port/include/log_macro.h -Add detailed description Add one missing description port/include/logger.h -Add detailed description -Add brief description [@file] Add missing descriptions Use \ingroup, \defgroup (if applicable) port/include/loggerstring.h -Add detailed description -Add brief description [@file] Document functions Use \ingroup, \defgroup (if applicable) port/include/logparams.h -Add detailed description -Add brief description [@file] Document functions port/include/m2n.h -Add detailed description -Add brief description [@file] Use appropriate formatting to make comments VISIBLE Add missing descriptions Use <code> tags for code entities port/include/platform_core_natives.h -Add detailed description -Add brief description [@file] Document functions port/include/port_atomic.h -Add detailed description -Add brief description [@file] Add missing descriptions -Use @return & @note port/include/port_disasm.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Define parameters as <in> / <out> port/include/port_dso.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers. port/include/port_filepath.h -Add detailed description -Add brief description [@file] Add missing descriptions Use \ingroup, \defgroup (if applicable) port/include/port_general.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/port_malloc.h -Add detailed description -Add brief description [@file] Document functions Use appropriate formatting port/include/port_sysinfo.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/port_timer.h -Add detailed description -Add brief description [@file] port/include/port_vmem.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers. -Define parameters as [in] / [out] port/include/stack_iterator.h -Use <code> tags for code entities port/include/tl/allocator.h -Add detailed description -Add brief description [@file] Use appropriate formatting to make comments VISIBLE port/include/tl/list_mt.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/tl/memory_pool.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/tl/set_mt.h -Add detailed description -Add brief description [@file] Add missing descriptions Use appropriate formatting port/include/tl/set_mt.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/tl/vector.h -Add detailed description -Add brief description [@file] Add missing descriptions port/include/tl/vector_mt.h -Add detailed description -Add brief description [@file] Add missing descriptions port/src/encoder/dec_base.h Add missing descriptions Use appropriate formatting to split code and comments port/src/encoder/enc_base.h Add missing descriptions Use <code> tags for code entities Define parameters as <in> / <out> -Add detailed description No need to put @brief in the beginning of function descriptions. Please remove. port/src/encoder/enc_defs.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Use appropriate formatting -Use @return & @note port/src/encoder/enc_prvt.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Use appropriate formatting port/src/encoder/encoder.h Add missing descriptions Use appropriate formatting Use \ingroup, \defgroup (if applicable) port/src/lil/em64t/pim/include/lil_code_generator_em64t.h -Add detailed description -Add brief description [@file] begin sentences with the capital letter. Use <code> tags for code entities port/src/lil/ia32/pim/include/lil_code_generator_ia32.h -Add detailed description -Add brief description [@file] Document functions port/src/lil/ipf/pim/include/lil_code_generator_ipf.h -Add detailed description -Add brief description [@file] Document functions Use appropriate formatting port/src/lil/em64t/pim/m2n_em64t_internal.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Avoid descriptions given in brackets. This info is perceived as redundant and excessive, and is often omitted by the readers. -Define parameters as [in] / [out] port/src/lil/ia32/pim/m2n_ia32_internal.h -Use appropriate formatting -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities port/src/lil/ipf/pim/m2n_ipf_internal.h -Add detailed description -Add brief description [@file] Add missing descriptions Use <code> tags for code entities Use appropriate formatting to make comments VISIBLE Component manager include/open/compmgr.h -Add detailed description (if applicable) Use <code> tags for code entities include/component_manager.h -Add detailed description -Add brief description [@file]
          Hide
          Svetlana Konovalova added a comment -

          2include_headers.patch corrects grammar and fixes formatting in include/open/compmgr.h and include/component_manager.h

          Here are suggestions on further improvement:

          include/open/compmgr.h

          • Define parameters as [in]/[out]
          • Add member data documentation descriptions
          • not sure it should be this way:
            "instance - a handle of an instance to free"
            Maybe <code>FREE</code>, or what?

          component_manager.h

          • add brief/detailed descriptions
          • define parameters as [in]/[out]

          Would be great if someone could take care of the aforementioned files.

          Thanks in advance,
          Sveta

          Show
          Svetlana Konovalova added a comment - 2include_headers.patch corrects grammar and fixes formatting in include/open/compmgr.h and include/component_manager.h Here are suggestions on further improvement: include/open/compmgr.h Define parameters as [in] / [out] Add member data documentation descriptions not sure it should be this way: "instance - a handle of an instance to free" Maybe <code>FREE</code>, or what? component_manager.h add brief/detailed descriptions define parameters as [in] / [out] Would be great if someone could take care of the aforementioned files. Thanks in advance, Sveta
          Hide
          Alexei Fedotov added a comment -

          Thank you for your corrections. Adding my comments concerning the fixes.

          1. What is the reason behind the following change?
          /**

          • @ingroup Handles
          • * The handle of the open component.
            + * The open component handle.
            */

          Probably "of" can be replaced with "for" in my description, and there should be undefined articles. I intentionally put the long form here to outline that this is a handle.

          2. What is rationale for the following change? License text is not a javadoc.

          -/*
          +/**

          • Licensed to the Apache Software Foundation (ASF) under one or more
          • contributor license agreements. See the NOTICE file distributed with

          3.
          /**

          • * If no component manager exist, initializes a component manager.
          • * Otherwise, increases a component manager reference count.
            + * Initializes a component manager if it does not exist.
            + * Otherwise, increases a component-manager reference count.

          The sentence becomes incorrect. The system won't initialize a component manager as well if another one exists. I asked Nadya to provide a reference to the non-Intel documentation which prefixes "managers" with "-".

          4.
          *

          • * @param init_func - initializer function which provides a default
            + * @param init_func - the initializer function providing a default
          • and private interfaces for the component
            An initializer function? Why we use "the" for non-defined term?

          5. I'm ok with all following changes
          /**

          • * Decrement a reference counter and destroy a component manager if it
            + * Decrements a reference counter and destroys a component manager if it
          • becomes zero. The caller should ensure no cached handles will be used.
            *

          6. It seems that the patch doesn't have correct line endings.
          -/**
          +
          +/**

          7. What is the reason behind this change?
          /**

          • @name Handles
          • * Handles are opaque for an interface user. The user
          • * operates with handles by means of interface functions.
            + * Handles are opaque for an interface user. The user operates with handles
            + * by means of interface functions.
            */

          8. A format is changed to the incorrect one.

          • /**
          • * Returns a component version which is numbers separated with dots.
          • * Implementors must check a major version number for compatibility.
          • */
            +/**
            + * Returns a component version where numbers are separated with dots.
            + * Implementors must check the major version number for compatibility.
            + */
          Show
          Alexei Fedotov added a comment - Thank you for your corrections. Adding my comments concerning the fixes. 1. What is the reason behind the following change? /** @ingroup Handles * The handle of the open component. + * The open component handle. */ Probably "of" can be replaced with "for" in my description, and there should be undefined articles. I intentionally put the long form here to outline that this is a handle. 2. What is rationale for the following change? License text is not a javadoc. -/* +/** Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with 3. /** * If no component manager exist, initializes a component manager. * Otherwise, increases a component manager reference count. + * Initializes a component manager if it does not exist. + * Otherwise, increases a component-manager reference count. The sentence becomes incorrect. The system won't initialize a component manager as well if another one exists. I asked Nadya to provide a reference to the non-Intel documentation which prefixes "managers" with "-". 4. * * @param init_func - initializer function which provides a default + * @param init_func - the initializer function providing a default and private interfaces for the component An initializer function? Why we use "the" for non-defined term? 5. I'm ok with all following changes /** * Decrement a reference counter and destroy a component manager if it + * Decrements a reference counter and destroys a component manager if it becomes zero. The caller should ensure no cached handles will be used. * 6. It seems that the patch doesn't have correct line endings. -/** + +/** 7. What is the reason behind this change? /** @name Handles * Handles are opaque for an interface user. The user * operates with handles by means of interface functions. + * Handles are opaque for an interface user. The user operates with handles + * by means of interface functions. */ 8. A format is changed to the incorrect one. /** * Returns a component version which is numbers separated with dots. * Implementors must check a major version number for compatibility. */ +/** + * Returns a component version where numbers are separated with dots. + * Implementors must check the major version number for compatibility. + */
          Hide
          Nadya Morozova added a comment -

          a couple of comments on questions from Alexei:

          1 - ok, the intention was presumably to make the phrase shorter; suggest that we stick with the original, "of" > "for" change is not recommended

          2 - we've talked about it : if you have a non-Doxygen comment, Doxygen does not parse it so when opening a Source view of the parsed file and expect to get a clean version, you see the disclaimer; on the other hand, if you format the disclaimer so that Doxygen can digest it, the comment disappears from the source view but is not attached to any code entity; using normal Doxygen comment notation for disclaimers has been a common practice for quite a while on Harmony

          3 - about incorrect wording - i disagree with Alexei; the phrase "Initializes a component manager if it does not exist. " can only be interpreted as it reads - if there has been no component manager, it is created; the new version of the phrase is shorter and does not repeat "component manager" two times

          3 - about compound nouns - indeed, the hyphen is widely used and helps avoid ambiguity; a quote from a well-known and respected book on technical writing Microsoft Manual of Style for Technical Publications: " Hyphenate two or more words that precede and modify a noun as a unit if confusion might otherwise result.
          Use memory-resident program, not TSR, in content for home users and information workers.
          Some programs have dialog box-type options for frequently used operations.
          Other examples include items such as "arguments, command line" and "command-line arguments."
          i don't think this is a discussion of English grammar, but if you have more questions, i'd be happy to help and consult. Really

          4. Because we define a specific parameter, it passes the name of a specific function, and everything specific is definite - hence, use a definite article If you said "an initializer function", you'd mean any, does not matter which -but it does, it's code right there.

          6. yes, we should probably run a check for dos2unix and tabs-to-spaces fixes before supplying any patch

          7. i haven't found differences in wording, so i presume, the change is done to limit comment-line length to 80 characters. Sveta, correct me if you had some other fix in mind.

          8. i guess it's for unification purposes; we don't usually indent the first comment line; this is not a strict rule, but whichever style you choose, please follow through the whole file. comments that are formatted inconsistently should be brought to a unified manner.

          Alexei,
          please let me know if you have more questions. What should we do with the patch - once you're satisfied with its quality?
          thanks,
          looking forward to your reply, Nadya

          Show
          Nadya Morozova added a comment - a couple of comments on questions from Alexei: 1 - ok, the intention was presumably to make the phrase shorter; suggest that we stick with the original, "of" > "for" change is not recommended 2 - we've talked about it : if you have a non-Doxygen comment, Doxygen does not parse it so when opening a Source view of the parsed file and expect to get a clean version, you see the disclaimer; on the other hand, if you format the disclaimer so that Doxygen can digest it, the comment disappears from the source view but is not attached to any code entity; using normal Doxygen comment notation for disclaimers has been a common practice for quite a while on Harmony 3 - about incorrect wording - i disagree with Alexei; the phrase "Initializes a component manager if it does not exist. " can only be interpreted as it reads - if there has been no component manager, it is created; the new version of the phrase is shorter and does not repeat "component manager" two times 3 - about compound nouns - indeed, the hyphen is widely used and helps avoid ambiguity; a quote from a well-known and respected book on technical writing Microsoft Manual of Style for Technical Publications: " Hyphenate two or more words that precede and modify a noun as a unit if confusion might otherwise result. Use memory-resident program, not TSR, in content for home users and information workers. Some programs have dialog box-type options for frequently used operations. Other examples include items such as "arguments, command line" and "command-line arguments." i don't think this is a discussion of English grammar, but if you have more questions, i'd be happy to help and consult. Really 4. Because we define a specific parameter, it passes the name of a specific function, and everything specific is definite - hence, use a definite article If you said "an initializer function", you'd mean any, does not matter which -but it does, it's code right there. 6. yes, we should probably run a check for dos2unix and tabs-to-spaces fixes before supplying any patch 7. i haven't found differences in wording, so i presume, the change is done to limit comment-line length to 80 characters. Sveta, correct me if you had some other fix in mind. 8. i guess it's for unification purposes; we don't usually indent the first comment line; this is not a strict rule, but whichever style you choose, please follow through the whole file. comments that are formatted inconsistently should be brought to a unified manner. Alexei, please let me know if you have more questions. What should we do with the patch - once you're satisfied with its quality? thanks, looking forward to your reply, Nadya
          Hide
          Alexei Fedotov added a comment -

          Nadya, thanks. The only remaining comment is that "an initializer function" sounds better for me than "specific initializer funciton".

          Show
          Alexei Fedotov added a comment - Nadya, thanks. The only remaining comment is that "an initializer function" sounds better for me than "specific initializer funciton".

            People

            • Assignee:
              Unassigned
              Reporter:
              Svetlana Konovalova
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development