Ian Collins wrote: > 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.
Ok... see below... > >>2 - Are there any unit tests? > > > >What do you mean with "unit tests" ? > > > Are you serious?? Erm, yes I am serious since I don't know the exact term "unit tests". I am not a native english speaker and try to be carefull before answering something completely wrong. With "unit tests" you mean "test suite for |wordexp()|", right ? > 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. If you're asking for a "test suite" for |libc::wordexp()|: glibc has one and April has another set of tests. But the more important issue is that it works propperly with SMF. This is the real "trap" where the various OpenSolaris distributions are suffering from and that's why we have it in the tree (and SMF is the only consumer of |libc::wordexp()| in Solaris and Google codesearch lists lots of implementations but no users...). > >>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. Yes, but my current focus is the putback of ksh93. The |libc::wordexp()| thing was tacked-on later because the OpenSolaris distributions need it desperately. > >>6 - 109, are multiple variables declared in one line acceptable? > > > >cstyle says "yes"... :-) > > Yuck. Why ? Sometimes it's usefull, sometimes it's just ugly. I've seen worse... MUCH worse things... =:-) > >>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. Tell that the original author of |wordexp()| ... :-) > >[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! Well, I wish Sun would make their tests OpenSource... but that is AFAIK one of the future "ToDo" items which come after our putback... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) [EMAIL PROTECTED] \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;) _______________________________________________ opensolaris-code mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
