Ian Collins wrote:
> 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.
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. An additional problems comes from SMF which uses
|libc::wordexp()| and fails in horrible ways if |libc::wordexp()|
crashes or produces the wrong output, usually resulting in a machine
which cannot change runlevels anymore (that's why it was requested to
have such a thing in the OS/Net codebase). That makes development not
really easy and that's why I am trying to avoid a rewrite of this horror
for now. I even went to far and did not fix obvious bugs in the old
version, e.g. in
-- snip --
328 wordexp(const char *word, wordexp_t *wp, int flags)
329 {
330 static char options[9] = "-";
331 static char *args[4];
332 const char *path;
-- snip --
... the |static| is AFAIK wrong in this case for a function which is
marked as "MT-Safe". I fixed this for the ksh93 version but not the old
one to make sure we do not introduce even the slightest risk of any
|libc::wordexp()|-related problems with this putback (yes, we did test
the new version extensively... but I'd like to say "...see, we didn't
touch the function which is used in the default build at all...").
> 2 - Are there any unit tests?
What do you mean with "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.
It's intentional to keep both versions of the |libc::wordexp()| function
around. Based on the experience with failling |libc::wordexp()| I'd say
that any other strategy would result in a high-risk putback and that's
the last thing I want.
> 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.
> 6 - 109, are multiple variables declared in one line acceptable?
cstyle says "yes"... :-)
> If so,
> to which one does the comment apply?
To all three variables. They track the buffer itself and two working
pointers .
> 7 - It would be clearer to declare loop counters where they are used.
See below.
> 8 - Why use short variable names that require a comment rather than a
> meaningful name?
See above... it's not a rewrite of the function. I just adopted it to
work with ksh93. I don't even dare to try a rewrite (for now [1]) since
getting it right is difficult and the testing all OpenSolaris
distributions (via email) would need several months (and I do not have
the test suites Sun has for this standards stuff... ;-( ).
[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).
----
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
