John Darrington <[email protected]> writes: > I think the ROC procedure is finished. > > Any reviews, before I merge to master are welcome.
In the documentation, a '=' is missing from the syntax summary for the PLOT subcommand. "consolidate" is spelled with an "i" :-) casereader_create_distinct seems to misuse the return value of casereader_get_proto. It assigns this (const) return value to a non-const variable, then modifies it (sometimes). To modify it, and to keep a reference to it in cdr->proto, it needs its own reference to it (with caseproto_ref). Then it needs to release that reference (with caseproto_unref) when cdr is destroyed. I don't really understand the algorithm used by casereader_create_distinct. Why are there two layers (casereader_create_filter_func and casereader_create_translator)? Why is it necessary to use casereader_peek (which forces all of the input data to stay around) instead of reading cases one at a time (and possibly being able to throw away data that has been consumed)? cmd_roc returns a literal value of 2 on error. Probably it should return CMD_FAILURE instead (which has value -2). cmd_roc returns a literal value of 1 on success. Probably it should return CMD_SUCCESS instead (which does have value 1). cmd_roc fails to free allocated memory (e.g. cmd.vars) on some errors. accumulate_counts will skip the first case if its cp value is SYSMIS. I don't know whether that is important. process_group does a *lot* of manipulation of cases and caseprotos and readers and writers. I didn't check it in detail because it wasn't obvious to me what the overall goal was. It could really use a high-level comment. It seems that there should be macros for 3 and 4 (following VALUE, N_EQ, N_PRED)? The literal values 3 and 4 are used in a number of places. The do_roc function could use some internal comments, that describe at least what each block of code is doing. roc.h doesn't declare anything that one can't get from command.h, so I would delete it. Besides, command.c doesn't include roc.h, so the prototype in roc.h doesn't provide the assurance that a prototype should (of making sure that all the callers see the correct function signature). I didn't review the correctness of the algorithms at all (nor am I familiar with what algorithms should be used). -- "Then, I came to my senses, and slunk away, hoping no one overheard my thinking." --Steve McAndrewSmith in the Monastery _______________________________________________ pspp-dev mailing list [email protected] http://lists.gnu.org/mailman/listinfo/pspp-dev
