[illumos-Developer] Fwd: [Urgend] Round eleven of code review for ksh93-integration+POSIX utility update3 ...

[Richard Lowe]

I'm still looking through actual changes, but decided to send this now
rather than in one long drop.  (It's looking likely that a bunch of
the stuff I hoped to look for someone else is already looking at)

- I'd like to see illumos bugs filed to match the relevant subset of bugs there

- You claim to be fixing tail(1), that's not going to be true of illumos

- When you merge with illumos, I'd like to see an incremental webrev
showing any conflicts you had to resolve.  I don't think that there
will be many, but I'd expect some:  webrev -p <this webrev> -o <new
webrev> should do it.  The same would apply when you re-generate the
headers for illumos, which you should do due to the wholesale i18n
changes made in illumos.

- Do the ast_pathaccess_YYYMMDD type changes reflect things which
should also be represented in symbol versions (mapfile-vers)?

- The files which change only in the comment indicating from where
they were generated (/home/gisburn/...) should be reset so that they
don't change at all.[1]

- Do you need manual page updates?  If so, you should make them.
  I see changes to the manual pages in the libshell source, but I
don't know how, if at all, they're involved in the actual shipping
pages.  If these pages should directly match the ones delivered, you
should probably drop them into the right places in usr/src/man

- If the code in /usr/demo/ksh is for reference, as is normal, there's
no reason to also deliver it compiled with shcomp

- It looks a whole lot like usr/demo/ksh has become a dumping ground

- Making _no_ changes except for copyright updates and "generated
from" lines sure does make this noisy. You should just not integrate
the new files, since they haven't actually changed in a way that
matters.[1]

- Why do same of the headers seem to have two copies _per-platform_
for a total of 8? (ast_stdio.h is the first I noticed)

  usr/src/lib/libast/i386/include/ast/ast_stdio.h
usr/src/lib/libast/i386/src/lib/libast/ast_stdio.h
  (The only difference between these two is that the latter lacks an
AT&T copyright notice)

on specific files and lines:

Makefile.ast:51

  lib/libshell/common/RELEASE says this is being evaluated (actually
"being evaluation."  I'm guessing the corrected text).  If this means
it might be removed, incompatibly altered or maybe that it's just
bleeding edge it might be better to skip it.

cmd/ast/msgcc/Makefile:54,60

  use $(RM) and $(CHMOD)

cmd/ksh/builtins/Makefile:88,
cmd/ast/msgcc/Makefile:36,
lib/libast/Makefile.com:1023

  Why is this not appended to the normal $(CPPFLAGS), or at least
derived from $(CPPFLAGS.master) or perhaps why doesn't it use
Makefile.ast and LIBSHELLCPPFLAGS?  Perhaps the comment in
libast/Makefile.com is trying to explain it, but I don't understand
why you think CPPFLAGS.master would have to be last, or what case
you're worrying about introducing silent breakage.

  It seems at the very least you could and should have ENVCPPFLAGS* in
here, or, in fact, not actually be _removing_ things that would be in
CPPFLAGS.master, even if you're correct that you can't use it, with
additions, directly (I'm not convinced).

cmd/ksh/builtins/alias.c

  To be clear, this is defining the things which _could_ be handled by
alias, and is not stealing the full set of commands defined in this
lookup table?

lib/libast/Makefile.com:693,713

  Is anyone working on fixing the warning problems here?

lib/libast/common/comp/conf.tab:391

  You could move this #endif all the way down through line 402.

lib/libast/*/include:

  I'd talked with Roland long, long ago about the headers in
per-platform directories that don't actually differ across platforms
(there were rather a lot), and how it would be better to put them
under common/ and share them between platforms, rather than having 4
(or sometimes 8, see above) identical copies.  Regardless of them
being generated, this shouldn't be that difficult.

lib/libast/common/stdio/getwc.c:27,
lib/libast/common/stdio/getwchar.c:27,
lib/libast/common/stdio/putwc.c:27,
lib/libast/common/stdio/putwchar.c:27

  Where did these parentheses come from?  Why?

lib/libast/common/string/fmtint.c

  A short comment describing this would be nice, so that it only
requires a brief glance.

  I was going to ask you how one was supposed to free the returned
buffer, since you may return a pointer somewhat arbitrarily far into
what fmtbuf() gives you, but then I saw fmtbuf:
http://src.illumos.org/source/xref/illumos-opengrok/usr/src/lib/libast/common/string/fmtbuf.c#41
which I'm having trouble forming words around.

lib/libast/common/string/strtoi.h:78,83

  Is this change in the base used for 'G', 'K' and 'M' user visible?
  What does it affect?

lib/libdll/mapfile-vers

  You turned this back into a version1 mapfile.  Do not.

lib/libshell/Makefile.doc,
lib/libsum/Makefile.com

  The only change in these files is the copyright date, and Sun ->
Oracle copyright.  Don't do that.

-- Rich

[1] In the interests of science, and because I had to do it _anyway_
to make this something it was possible to review, I present here
diffstat output comparing the original diff with a diff from which I
have removed files that change *only* in copyright year and/or
"generated from" (such that we take the existing, functionally
equivalent copy instead):

before:
  1964 files changed, 41986 insertions(+), 25778 deletions(-)
after:
  782 files changed, 40708 insertions(+), 24499 deletions(-)

And keep in mind that many of these files are present in 4 (or 8)
identical copies, so likely the "after" copy would be much more
manageable with that tidied too.