ToolsReviewFeedback
From Genunix
Howdy, and thanks for reviewing these tools.
Please add an entry for yourself in the Code Reviewers table, then sort your comments into the three broad buckets below. Within each bucket, please organize your issues by file.
Please tag your comments per your entry in the Code Reviewers table, so that we can follow up if we have questions. Feel free to add your tag to an issue that somebody else has already reported, if you feel it should be emphasized.
For our part, we'll be adding notes describing resolution for these issues.
| Name | Tag |
|---|---|
| Mike Kupfer | MK |
| Mark J. Nelson | MJN |
| Jim Carlson | JC |
| Rich Lowe | RL |
| James McPherson | JMCP |
| Danek Duvall | DD |
| Dan Price | DP |
| Darren Moffat | DJM |
| Will Fiveash | WF |
Contents |
[edit]
Must fix prior to putback
[edit]
Nits and niceties
MJN: Anything left in this section after the tools putback will be triaged and dealt with.
- usr/src/tools/scripts/hdrchk.1
[MK] HEADER STANDARDS: I thought we had a separate writeup of the
header file standards, but I can't easily find it from the ON
Community web page. (Maybe it's in the ON Developer
Reference?) If we do have a separate writeup, the hdrchk man
page should give its URL, rather than duplicating the
information.
RL: There's a description in the C Style Guide, and one (mingled in with bits of cstyle information) in 7.2 of the devref.
The former is the only other complete description I know of beyond the comments in hdrchk itself.
MJN: If somebody provides a pointer, we can adjust this later, but I don't know where to find one.
- usr/src/tools/onbld/Checks/Comments.py
[RL] 108/116: The use of check_db here could be clearer.
comment [MJN]: Seems pretty clear to me, if we fix it, it should probably be via doc string.
[WF] 40: Note that wx is using:
arc='(FW|LS|PS)ARC[\/ ][12][0-9][0-9][0-9]\/[0-9][0-9][0-9][^0-9]'
which is more restrictive. I was told when making this change that
it was preferable to restrict the ARC regex matches to just FWARC,
LSARC or PSARC since this list was not likely to change often and it
was better to catch a misspelled ARC comment.
comment [MJN]: Reasonable to squeeze this in with other stuff, but not a realistic source of problems.
(Even though I probably made the comment to which Will referred in the first place.)
[edit]
Post-tools-integration followup issues
- usr/src/tools/onbld/Checks/DbLookups.py
[JC] It'd be nice if this knew about defect.opensolaris.org. Lack
of direct support of open bug tracking in the new tools is
going to be a stumbling block for external committers.
DOODiscussion
[RL] 122: I still really want BooBug and Monaco to share a common
interface, rather than papering over it in BugDB. I think
it'd be considerably more tidy
- usr/src/tools/onbld/Checks/Keywords.py
[JC] 44: this will accidentally match (and identify as an "SCCS
keyword") perfectly valid things such as:
printf("foo %X%s", hexval, "\n");
I'm not sure what to do about that problem, but I think that
as we get further and further away from SCCS, we will want
to rethink this.
KeywordsDiscussion
- usr/src/tools/onbld/Checks/Rti.py
[JC] 91: I think this could be more efficient. Querying an RTI
should return a list of CRs (all of the CRs in the RTI),
and we should be able to check against that list. Most
(all?) putbacks have exactly one RTI. In fact, querying
the database and finding multiple different RTIs for the
CRs the user lists is probably an error case to flag.
filed 496 DbLookups should consider following RTIstatus with RTIget
- usr/src/tools/onbld/Checks/HdrChk.py
[JC] 46,146: this isn't needed once we get past Teamware.
- usr/src/tools/scripts/nightly.1
[RL] ... Could note that BRINGOVER_FILES is similarly TeamWare specific, except that we don't document that in nightly(1) at all (it's somewhere, where?)
- usr/src/tools/scripts/nightly.sh
[JC] 1707: uuoc
UUoCDiscussion
[RL] 2109: This bug (186), may now be fixed in mercurial?
comment: MJN: Yes, issue 186 is resolved, and provides a way to
check if the tip of the parent is present in this workspace, but
who actually thinks it's a good idea to do an automatic bringover
in a workspace that might contain local changes?
- usr/src/tools/scripts/rtichk.py
- usr/src/tools/scripts/copyrightchk.py
- usr/src/tools/scripts/hg-active.py
[RL] See bug 479 bin versus lib; man pages
MJN: I plan to fix our stuff and the preexisting stuff at once post-tools-putback
- usr/src/tools/scripts/hg-active.py
[RL] 73: Should this accept paths other than the workspace root?
- usr/src/tools/scripts/webrev.1
[RL] See bug 208 *webrev* the manpage contains some typos/syntax buglets
MJN: this bug is assigned to darrenm, and is not urgent
[JC] 140: just a style nit: I think \fP is preferred over \fR, so that
attributes can nest.
MJN: I wouldn't mind seeing a style guide; my .man memory got paged out many years back.
In any event, I'm deferring this. After the tools putback, I'll triage a general
tools manpage cleanup effort for this.
- usr/src/tools/scripts/webrev.sh
[RL] See bug 209 *wdiff* and *webrev* print misaligned line numbers for >= 10000 lines
MJN: This one is assigned to darrenm, and can be cleaned up after the tools putback
if he doesn't want to continue to own it.
[DP] Is the change at 1216 (eval) stylistic, or is there an effect?
RL: This change came in rev: 5576:0eb2a8c6ec3a, which is darrenm's initial putback.
I can't currently say I know why it was made.
I don't immediately see why behaviour would change. But in that case, why the change?
[DP] 1493: The last... 20 or so nevada builds also have /usr/bin/stat, which seems like a nice tool
for doing this.
MJN: Will triage post-tools-putback.
- usr/src/tools/onbld/Checks/CStyle.py
[DD] 50: is "name" going to be present for file-like objects that don't actually represent files?
RL: not necessarily, but in that case you should be passing a name via the filename arg. (perhaps we should force that in general).
Either way, we actually want to crash if we have neither, so rectifying this would be explicitly raising rather than letting it crash.
- usr/src/tools/onbld/Scm/WorkSpace.py
[RL] 362: This does a bunch of work by hand it really shouldn't need
to, we already know the revisions in the AL, we can find
the ones with a single parent that is not itself in the AL
easily enough without having to walk through history.
Ordering would need to be looked into carefully, however.
[JC] 515: this seems to occur in a lot of cases, including just listing
the active files. Something likely eventually needs to be done
about this. (Not sure if it's a cache or something else.)
I'll admit I didn't read a lot of the rest of the code. I
frankly don't understand it.
[RL] 651: Hardly code review, but I sure do wish this took the same
args as diff.
* usr/src/tools/onbld/Checks/__init__.py
[RL] 34: Is __all__ correct here?
MJN: Probably not, since it leaves out the exceptions and some of the classes
RL: We intend it to skip internal things that shouldn't be pulled in via 'from onbld.Checks import *'
[RL] 57: onSWAN() should catch a specific exception, or set thereof
MJN: Probably, but for now, a stack trace should implicate socket,
thus giving us a pretty good idea where to look. Especially since
there is exactly one line of code that could possibly throw an
exception in the whole method.
* usr/src/tools/onbld/Scm/Backup.py
[RL] 350: I think John Levon had concerns about us including Mq
patches in this
* usr/src/tools/onbld/hgext/cdm.py
[RL] 24: See bug 303 cdm needs .NOT file equivalents
[RL] 107: Hardly code review, but I sure do wish this took the same
args as diff.
[RL] 285: Do we care about this, as compared to cdm_list?
MJN: line number is off; RL is asking about hg renamed
[DD] 441-446: seems like this could be usefully refactored into its own method.
RL: Checks.Comments should really give us a way to pull this stuff out.
[edit]
Not yet triaged
[edit]
Resolved
There's too much to fit here, so it's now on a separate ToolsReviewResolutions page.
