Roland Mainz wrote: >* Webrev over all non-AST files (this includes the files in >usr/src/cmd/ast/msgcc/ my accident): >http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/non_ast_files/webrev/ > > > A couple of comments on wordexp.c:
1 - the flow of wordexp would be easier (OK, possible!) to follow if the code in each large conditional block were factored out into functions. At the very least, the parent and child paths should have their own functions. 2 - Are there any unit tests? 3 - Following point 1, the structure of the function could be improved to eliminate the use of goto. 4 - There is an incredible amount of duplicated code between the two versions of wordexp, following point 1 would enable more reuse and make the differences between the two versions clearer. 5 - The duplication highlighted in point 4 will be a maintenance headache. 6 - 109, are multiple variables declared in one line acceptable? If so, to which one does the comment apply? 7 - It would be clearer to declare loop counters where they are used. 8 - Why use short variable names that require a comment rather than a meaningful name? Cheers, Ian. _______________________________________________ opensolaris-code mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
