Roland Mainz writes:
> * 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/
I did some light review of this. Some comments:
cmd/ast/msgcc/Makefile
56-66: why are these needed? Don't the default targets work
properly? The usual way to do this would be:
PROG= msgcvt msggen msgget msgcpp
and let make figure out the implicit dependencies. Is there some
reason that doesn't work here?
69-75,78-83: consider creating a common build rule for these.
91-92: what is this? This ought o be a part of the Targetdirs.
99: I'd suggest a dependency rather than .WAIT.
105-110: why is this needed? What's wrong with the default rules?
cmd/ksh/amd64/Makefile
cmd/ksh/i386/Makefile
39-48: use the $(INS.link) rule instead.
cmd/ksh/Makefile
58-61: why is i18n dummied out?
66-73: looks like $(INS.link).
cmd/ksh/Makefile.com
99-101: why is a custom build rule needed? Why not just set up a
dependency?
100: yuck.
105,120: s/expiclitly/explicitly/
108: "seems to require"?
126-131: this is what Makefile.lint is about; just remove cmd/ksh
from the COMMON_SUBDIRS list (it's not there now, by the way), and
all should be fine. This extra bit of work shouldn't be needed.
lib/libast/Makefile
159: I think you might want just "FRC" here, though it's not clear.
The current build rule will cause make to execute the $(SUBDIRS)
build rule with an empty $(TARGET) -- building whatever is the
default target in $(MACH).
lib/libast/i386/Makefile
34: what is this about? What do Solaris distributors need to do
when updating the minor release number? (And why doesn't the major
release number fit in here somewhere?)
47-60: what's the status of the real fix? (I don't really follow
why this is a special problem for ksh93 and doesn't seem to affect
anything else in ON -- all of which, varargs and not, goes through
gcc shadow compilation. Is the bug actually related to the new
AST-specific varargs?)
lib/libast/Makefile.astinclude
28-29: is this true? (And if it is, how does the comment help
explain the code that follows?)
32-38: I don't follow this. What exactly is wrong with _LP64? It's
set correctly by sys/isa_defs.h -- why would you need to do your own
version of this logic?
40: What's the justification for having include files that are
dependent on architecture? This seems too complex by at least half
to me.
56: s/concaternation/concatenation/
lib/libast/Makefile.com
33-602: suggest condensing this list with appropriate build rules.
612,616: these shouldn't be necessary, I think.
646-650: I don't really follow. Can this be reworded?
650: s/expliclity/explicitly/
653: what does "be careful" mean here?
653: s/carefull/careful/
682-708: not sure that the comments add much value here.
709: that's worrisome.
715-720: not really needed.
lib/libast/Makefile.libastl10n
36-37: this has to go. We can't support LD_LIBRARY_PATH hacks that
attempt to run software out of the proto area.
lib/libast/mapfile-vers
29-618: unhelpful comments; find another place to document the
history.
lib/libcmd/i386/Makefile
35: please don't add stray blank lines at the end of the file.
lib/libcmd/sparcv9/Makefile
28: not sure why this is needed.
lib/libc/port/regex/wordexp.c
general: please read the C Style guidelines; all ON code (other than
ported-in stuff) is expected to comply.
http://opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms.pdf
79,80: put on one line
81: arguments could be const
83,84: these argument copies aren't needed.
86,87: opening brace goes on same line.
104: what does "10" represent?
139: consider using "!(flags & WRDE_DOOFFS)" instead; it's more
legible that way. (Also true of the other flag tests.)
150: don't cast the result of malloc; the type is already
compatible.
151: indenting is off.
155: you've allocated (wptmp.we_offs + INITIAL) pointers, but you're
initializing only the first (wptmp.we_offs) of them. Is this
intentional? (If so, it deserves a comment.)
155-156: either memset or bzero would do the job.
157: this looks like a boolean_t to me.
185: consider using a #define or some other mechanism rather than a
hard-coded "124" here.
195: I'm confused. This code is apparently intended to be compiled
only on systems where /usr/bin/ksh is identical to ksh93. So, why
is "/usr/bin/ksh93" needed here?
255: BUFSZ doesn't look related to the work being done here. Is it
just a "convenient constant?"
298,300: if one arm of the 'if' statement needs braces, then both
do.
308,310,312: common #defines for these would be nice. (Yes, I know
that they're missing in the current code as well.)
lib/libshell/misc/buildksh93.readme
33: s/it's/its/
71: missing "##"?
pkgdefs/SUNWarc/prototype_com
75-76,113-114,182-183: something seems out of sync here. If you're
not shipping the compilation symlinks (the lib*.so symlinks), then
why ship the lint libraries? How is linting source that cannot be
linked useful?
(See SUNWcsl; it has only the *.so.1 files, not *.so links.)
pkgdefs/SUNWarc/prototype_i386
pkgdefs/SUNWarc/prototype_sparc
As above; can't tell why lint libraries are shipped.
pkgdefs/SUNWastdev/depend
Curious: how was this list determined?
pkgdefs/SUNWastdev/Makefile
32: you have your own 'depend' file. There shouldn't be a
dependency on it here! Please remove, as it'll cause source-only
builds to fail.
pkgdefs/SUNWcsu/prototype_i386
106-107: alphabetic order, please.
pkgdefs/SUNWcsu/prototype_sparc
55-56: alphabetic order.
pkgdefs/SUNWhea/prototype_com
165-166: I think 's' comes before 't', so 'assert.h' comes before
'ast'.
pkgdefs/SUNWosdem/prototype_com
60-99: alphabetic order, please.
Targetdirs
With the exception of line 223, all of the new entries in this file
look wrong to me. The "REALPATH" goop is needed for libraries that
are in /lib (in the root file system). That's not true of *ANY* of
the new AT&T libraries. They're all over in /usr/lib. Thus, no
magic "../../lib/" symlink (referring to the root file system) is
needed.
How does the software even build with these rules present? (It
should fail ... but I guess it's possible that there are bugs that
prevent it from failing ...)
--
James Carlson, Solaris Networking <james.d.carlson at sun.com>
Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
