Ticket #1726 (assigned todo)

Opened 4 years ago

Last modified 4 years ago

Missing POD in .pmc files (and a couple of others)

Reported by: mikehh Owned by: jkeenan
Priority: normal Milestone:
Component: coding_standards Version: trunk
Severity: medium Keywords:
Cc: Language:
Patch status: applied Platform:

Description

We recently completed an exercise whereby the coding standard test t/codingstd/c_function_docs.t and the associated documentation was fixed so that no TODO's remain for missing POD in c functions in parrot.

I had occasion to run make headerizer and found that it reported missing POD in some other files, mostly .pmc files and one .y file (plus 3 .c files from compilers/src/pirc not tested by c_function_docs.t).

It is suggested that .pmc files be included in c_function_docs.t or possibly in a new test.

at r48245 make headerizer furnishes the following output:

compilers/imcc/imcc.y: adv_named_set_u has no POD
compilers/pirc/src/main.c: process_file has no POD
compilers/pirc/src/pircompunit.c: set_sub_multi_types has no POD
compilers/pirc/src/pirparser.c: YYID  has no POD
src/pmc/bigint.pmc: bigint_init has no POD
src/pmc/bigint.pmc: bigint_clear has no POD
src/pmc/bigint.pmc: bigint_set has no POD
src/pmc/bigint.pmc: bigint_set_str has no POD
src/pmc/bigint.pmc: bigint_get_self has no POD
src/pmc/bigint.pmc: bigint_set_self has no POD
src/pmc/bigint.pmc: bigint_get_long has no POD
src/pmc/bigint.pmc: bigint_get_bool has no POD
src/pmc/bigint.pmc: bigint_get_string has no POD
src/pmc/bigint.pmc: bigint_get_double has no POD
src/pmc/bigint.pmc: bigint_add_bigint has no POD
src/pmc/bigint.pmc: bigint_add_bigint_int has no POD
src/pmc/bigint.pmc: bigint_sub_bigint has no POD
src/pmc/bigint.pmc: bigint_sub_bigint_int has no POD
src/pmc/bigint.pmc: bigint_mul_bigint has no POD
src/pmc/bigint.pmc: bigint_mul_bigint_int has no POD
src/pmc/bigint.pmc: bigint_pow_bigint_int has no POD
src/pmc/bigint.pmc: int_check_divide_zero has no POD
src/pmc/bigint.pmc: bigint_check_divide_zero has no POD
src/pmc/bigint.pmc: bigint_div_bigint has no POD
src/pmc/bigint.pmc: bigint_div_bigint_int has no POD
src/pmc/bigint.pmc: bigint_fdiv_bigint has no POD
src/pmc/bigint.pmc: bigint_fdiv_bigint_int has no POD
src/pmc/bigint.pmc: bigint_mod_bigint has no POD
src/pmc/bigint.pmc: bigint_mod_bigint_int has no POD
src/pmc/bigint.pmc: bigint_cmp has no POD
src/pmc/bigint.pmc: bigint_cmp_int has no POD
src/pmc/bigint.pmc: bigint_abs has no POD
src/pmc/bigint.pmc: bigint_neg has no POD
src/pmc/callcontext.pmc: ensure_positionals_storage has no POD
src/pmc/callcontext.pmc: ensure_positionals_storage_ap has no POD
src/pmc/callcontext.pmc: get_cell_at has no POD
src/pmc/callcontext.pmc: autobox_intval has no POD
src/pmc/callcontext.pmc: autobox_floatval has no POD
src/pmc/callcontext.pmc: autobox_string has no POD
src/pmc/callcontext.pmc: autobox_pmc has no POD
src/pmc/callcontext.pmc: get_hash has no POD
src/pmc/callcontext.pmc: mark_cell has no POD
src/pmc/callcontext.pmc: mark_positionals has no POD
src/pmc/callcontext.pmc: mark_hash has no POD
src/pmc/callcontext.pmc: get_named_names has no POD
src/pmc/class.pmc: pointer_compare has no POD
src/pmc/class.pmc: key_hash_pointer has no POD
src/pmc/class.pmc: cache_class_attribs has no POD
src/pmc/class.pmc: build_attrib_index has no POD
src/pmc/class.pmc: init_class_from_hash has no POD
src/pmc/class.pmc: initialize_parents has no POD
src/pmc/class.pmc: initialize_parents_pmc has no POD
src/pmc/class.pmc: make_class_name has no POD
src/pmc/class.pmc: calculate_mro has no POD
src/pmc/complex.pmc: int_check_divide_zero has no POD
src/pmc/complex.pmc: float_check_divide_zero has no POD
src/pmc/complex.pmc: complex_check_divide_zero has no POD
src/pmc/coroutine.pmc: print_sub_name has no POD
src/pmc/eval.pmc: clear_fixups has no POD
src/pmc/eval.pmc: get_sub has no POD
src/pmc/eval.pmc: mark_subs has no POD
src/pmc/fixedintegerarray.pmc: auxcmpfunc has no POD
src/pmc/hashiterator.pmc: advance_to_next has no POD
src/pmc/imageio.pmc: GET_VISIT_CURSOR has no POD
src/pmc/imageio.pmc: SET_VISIT_CURSOR has no POD
src/pmc/imageio.pmc: INC_VISIT_CURSOR has no POD
src/pmc/imageio.pmc: create_buffer has no POD
src/pmc/imageio.pmc: ensure_buffer_size has no POD
src/pmc/imageio.pmc: INFO_HAS_DATA has no POD
src/pmc/imageio.pmc: id_list_get has no POD
src/pmc/imageio.pmc: visit_todo_list_thaw has no POD
src/pmc/imageio.pmc: visit_todo_list_freeze has no POD
src/pmc/imageiosize.pmc: visit_todo_list_freeze has no POD
src/pmc/integer.pmc: maybe_throw_overflow_error has no POD
src/pmc/integer.pmc: upgrade_self_to_bignum has no POD
src/pmc/namespace.pmc: add_to_class has no POD
src/pmc/namespace.pmc: ns_insert_sub_keyed_str has no POD
src/pmc/namespace.pmc: maybe_add_sub_to_namespace has no POD
src/pmc/namespace.pmc: add_nci_to_namespace has no POD
src/pmc/namespace.pmc: add_multi_to_namespace has no POD
src/pmc/nci.pmc: pcc_params has no POD
src/pmc/nci.pmc: build_func has no POD
src/pmc/null.pmc: null_pmc_access has no POD
src/pmc/object.pmc: get_attrib_index has no POD
src/pmc/object.pmc: get_attrib_index_keyed has no POD
src/pmc/object.pmc: find_cached has no POD
src/pmc/object.pmc: cache_method has no POD
src/pmc/orderedhash.pmc: get_list_item has no POD
src/pmc/orderedhash.pmc: find_bounds has no POD
src/pmc/orderedhash.pmc: box_string has no POD
src/pmc/orderedhash.pmc: box_integer has no POD
src/pmc/orderedhash.pmc: box_number has no POD
src/pmc/packfile.pmc: copy_packfile_header has no POD
src/pmc/role.pmc: init_role_from_hash has no POD
src/pmc/sub.pmc: print_sub_name has no POD
src/pmc/threadinterpreter.pmc: stop_GC has no POD
Headerization complete.

Attachments

bigint Download (5.7 KB) - added by jkeenan 4 years ago.
Example of new output: perl t/codingstd/pmc_docs.t src/pmc/bigint.pmc
excerpts.parrotsketch.20100817.txt Download (3.8 KB) - added by jkeenan 4 years ago.
Aug 17 2010 #parrotsketch discussion on coding standards for documentation of C functions
c_function_codingstd.extract.txt Download (1.0 KB) - added by jkeenan 4 years ago.
Extract of docs/pdds/pdd07_codingstd.pod re documentation of C functions

Change History

in reply to: ↑ description   Changed 4 years ago by jkeenan

  • status changed from new to assigned

Replying to mikehh:

at r48245 make headerizer furnishes the following output:

> compilers/imcc/imcc.y: adv_named_set_u has no POD
> compilers/pirc/src/main.c: process_file has no POD
> compilers/pirc/src/pircompunit.c: set_sub_multi_types has no POD
> compilers/pirc/src/pirparser.c: YYID  has no POD
...

mikehh: Are these has no POD messages useful from the standpoint of someone working with make headerizer? Or would we be better off moving them entirely to the output of a new t/codingstd/pmc_docs.t file?

Thank you very much.

kid51

  Changed 4 years ago by mikehh

I don't think that that particular message (... has no POD) is of particular relevance to make headerizer (could be wrong ... comments?) but having a specific test for it would be a good thing.

We could have a TODO list initially as we did with c_function_docs.t and a Wiki entry, until we have the entries done.

Cheers, Michael (mikehh)

  Changed 4 years ago by jkeenan

Created branch tt1726_pmc_pod to work on this TT. Initial objective will be to create a t/codingstd/pmc_docs.t that works somewhat like c_function_docs.t.

kid51

  Changed 4 years ago by jkeenan

mikehh:

Can you review t/codingstd/pmc_docs.t in the tt1726_pmc_pod branch?

I began by cloning t/codingstd/c_function_docs.t. It went remarkably smoothly. (Why didn't we do this a long time ago?) pmc_docs.t detected 3 more .pmc files lacking documentation than did headerizer.t.

But, of course, I couldn't leave it there. I decided to do some refactoring of the internals of pmc_docs.t to create a hash in which the documentation status of each function in each .pmc file was recorded. Then the tests amounted to iterating through this hash. I also made a few modifications to the output that will appear under prove -v.

Thank you very much.

kid51

Changed 4 years ago by jkeenan

Example of new output: perl t/codingstd/pmc_docs.t src/pmc/bigint.pmc

  Changed 4 years ago by mikehh

Ran the test with prove -v in the branch - I did notice 3 boilerplate only functions and that src/pmc/bigint.pmc is the biggest culprit, followed by maybe src/pmc/callcontext.pmc

23 TODO'd tests out of 95.

In terms of removing the missing POD message in make headerizer, if we have the test its is fine for pmc files, but I think we should retain the message for other files.

Cheers, Michael (mikehh)

  Changed 4 years ago by jkeenan

  • patch set to applied

I have merged the tt1726_pmc_pod branch into trunk. This adds t/codingstd/pmc_docs.t which works very much like t/codingstd/c_function_docs.t, viz., a __DATA__ section in the test file holds a list of .pmc files which as yet have functions which are either entirely undocumented ('missing') or which just have POD =item placeholders ('boilerplate').

As time permits, I am going to do an svn annotate on these .pmc files and try to figure out who would be most qualified to write documentation on particular functions. I will then email the lucky individuals to request that they write documentation.

Keep your fingers crossed!

(Though the branch has been merged, this ticket will remain open until we clear up this backlog of missing documentation. Mike, if you would like to put this list on a wiki page, please go right ahead. I have not yet made any changes in tools/build/headerizer.pl; those can wait for what I have to do in TT #1725.)

Thank you very much.

kid51

$ perl t/harness t/codingstd/pmc_docs.t 
t/codingstd/pmc_docs.t .. 1/95 
not ok 8 - src/dynpmc/rational.pmc # TODO Missing function docs
not ok 13 - src/pmc/bigint.pmc # TODO Missing function docs
not ok 14 - src/pmc/bignum.pmc # TODO Missing function docs
not ok 17 - src/pmc/callcontext.pmc # TODO Missing function docs
not ok 19 - src/pmc/class.pmc # TODO Missing function docs
not ok 21 - src/pmc/complex.pmc # TODO Missing function docs
not ok 23 - src/pmc/coroutine.pmc # TODO Missing function docs
not ok 26 - src/pmc/eval.pmc # TODO Missing function docs
not ok 34 - src/pmc/fixedintegerarray.pmc # TODO Missing function docs
not ok 40 - src/pmc/hashiterator.pmc # TODO Missing function docs
not ok 42 - src/pmc/imageio.pmc # TODO Missing function docs
not ok 43 - src/pmc/imageiosize.pmc # TODO Missing function docs
not ok 45 - src/pmc/integer.pmc # TODO Missing function docs
not ok 52 - src/pmc/namespace.pmc # TODO Missing function docs
not ok 53 - src/pmc/nci.pmc # TODO Missing function docs
not ok 54 - src/pmc/null.pmc # TODO Missing function docs
not ok 55 - src/pmc/object.pmc # TODO Missing function docs
not ok 58 - src/pmc/orderedhash.pmc # TODO Missing function docs
not ok 60 - src/pmc/packfile.pmc # TODO Missing function docs
not ok 80 - src/pmc/role.pmc # TODO Missing function docs
not ok 90 - src/pmc/sub.pmc # TODO Missing function docs
not ok 92 - src/pmc/threadinterpreter.pmc # TODO Missing function docs
not ok 95 - src/pmc/unmanagedstruct.pmc # TODO Missing function docs
t/codingstd/pmc_docs.t .. ok     

  Changed 4 years ago by jkeenan

As of r48406, 3 down, 20 to go. Have emailed 4 contributors requesting PMC function documentation.

Thank you very much.

kid51

in reply to: ↑ description ; follow-up: ↓ 10   Changed 4 years ago by plobsing

Replying to mikehh:

Non-vtable functions in .pmc files are usually (always?) static. This means they are of no use to anyone outside of these files (eg: the user reading the docs).

What purpose does documenting these functions, which are little more than implementation details, with POD serve?

  Changed 4 years ago by mikehh

That is all very well, but at present, we at least have a test that determines if there is documentation related to a given function. Both c_function_docs.t and pmc_docs.t check to see if there is a header (=item) for each function and if there is some documentation there.

With these tests we at least can say that there is function documentation. This helps both the maintainer and the user, even if the user does not need the static function documentation, the maintainer does.

Unfortunately the test cannot determine the quality of the documentation, just its existence.

If there are any ideas as to ensuring the existence of documentation in another way I would be glad to hear them.

Cheers, Michael (mikehh)

in reply to: ↑ 8   Changed 4 years ago by jkeenan

Replying to plobsing:

Replying to mikehh:

What purpose does documenting these functions, which are little more than implementation details, with POD serve?

I second mikehh's remarks. Going into this exercise, the overwhelming majority of static functions in the .pmc files had neither comments nor POD. There's no point in worrying about signal-to-noise ratios if the signal is inherently faint.

Let's err on the side of completeness. Once all the functions are documented, we can revisit the question of the optimum balance between inline comments and POD.

Thank you very much.

kid51

  Changed 4 years ago by jkeenan

mikehh:

I believe that all of the files cited now have POD =item markers, so documentors will know where to insert the documentation.

I have written to about 10 of our contributors asking them to add function documentation to files which (per svn annotate) they have previously worked on. NotFound++ for responding promptly to my requests on two separate files.

As mentioned on IRC, t/codingstd/pmc_docs.t is an imperfect tool. (I'd give it a B-.) It has problems with function signatures that are long (>80? >100?) characters. So on some files where I have inserted POD formating markers, the tool still rates those functions as missing rather than boilerplate only. Perhaps this is because I had to break some of those =item lines so that the file could pass the linelength standard.

But we don't need to bother with all that until the release is out the door.

Thank you very much.

kid51

Changed 4 years ago by jkeenan

Aug 17 2010 #parrotsketch discussion on coding standards for documentation of C functions

Changed 4 years ago by jkeenan

Extract of docs/pdds/pdd07_codingstd.pod re documentation of C functions

  Changed 4 years ago by jkeenan

This ticket has been effectively stalled for two-and-a-half months. The approach taken by mikehh and me in our work on the ticket met strong opposition in #parrotsketch on August 17. I am attaching an extract of that discussion to this ticket.

I am also attaching an extract from docs/pdds/pdd07_codingstd.pod that is relevant to this discussion. Whatever we do in this area has to be governed by the PDD. The PDD calls for inline Pod documentation containing information on the implementation decisions. It goes on to prescribe a format for POD inside of C-style comments for [e]very non-local named entity, be it a function, variable, structure, macro or whatever. So it seems that our current policy leans in favor of documentation in POD format for C functions.

If you disagree with this policy, then a patch to revise pdds/pdd07_codingstd.pod needs to be proposed. And we will need patches for t/codingstd/c_function_docs.t and t/codingstd/pmc_docs.t that reflect the policy and report accurately on what remains to be documented.

Thank you very much.

kid51

Note: See TracTickets for help on using tickets.