Roland Mainz wrote: >Ian Collins wrote: > >>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. >> >> > >Well, the new version of |libc::wordexp()| is just a modified vesion of >the old |libc::wordexp()| code. I didn't tried to rewrite it since it >has so many sideeffects and problems that getting it "right" is >difficult. > >
Standard problem with legacy code, that's why I asked about unit tests. My approach is to add unit tests to the existing code to mitigate the risk when the code is refactored. >>2 - Are there any unit tests? >> >> > >What do you mean with "unit tests" ? > > > Are you serious?? Call me pig headed, but in my shop, code never gets written or changed before there are unit tests for it. If nothing else when working with legacy code, tests can be used to document the expected behaviour of the code under test. > > > >>5 - The duplication highlighted in point 4 will be a maintenance headache. >> >> > >AFAIK no since at some point we get rid of the old one. > > Sooner rather than later, I hope. > > >>6 - 109, are multiple variables declared in one line acceptable? >> >> > >cstyle says "yes"... :-) > > Yuck. > > >>If so, >>to which one does the comment apply? >> >> > >To all three variables. They track the buffer itself and two working >pointers . > > > Then neither the names or the comment are very helpful. > >[1]=Note: Am am planning to rewrite the function from scratch at a later >date after this putback... the current version is only a quick solution >to help non-Solaris OpenSolaris distributions to use SMF without having >to rely on the old ksh stuffed in /usr/bin/ksh (and to help us >collecting data for the /usr/bin/ksh migration to ksh93). > > > When you do, don't forget the tests! Ian _______________________________________________ opensolaris-code mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
