Ali Bahrami writes: > James Carlson wrote: > > General: > > > > There should be a prototype mapfile in usr/src/prototypes/. > > I created usr/src/prototypes/prototype.mapfile-vers
OK; thanks. Odd that this new prototype file contains this comment: # Generic interface definition for usr/src/cmd/sgs/libld. Not all new mapfiles should have this, should they? > Also, I drew a line at renaming existing mapfiles, mainly for scope > creep reasons. Having looked at all these files, I think that the > right policy is not to put too much faith in the precise name of > the mapfile. > > I went on about this in my response to Mark as well, so I'll stop > here. OK. > > Also, I note that you've changed the copyright on all of these > > files. Maybe it's too late now, but my previous understanding was > > that copyright dates are not supposed to be changed if only trivial > > changes (such as changes to comments) are made, and *most* of these > > changes are in fact trivial. > > I've not heard that. My understanding is, if you touch it, you > update the copyright. Not quite so. Here are some references: http://onnv.sfbay/copyright-policy.html http://mail.opensolaris.org/pipermail/companion-discuss/2006-October/000968.html The year changes only with "significant modification." Otherwise, we're claiming a renewed copyright when we haven't created anything new, and that's ungood. (_Maybe_ this is "documentation" and that'd be the slender reed that you use to justify the mass change ...) > I think copyrightchk enforces this, and > isn't it the rule that 'hg pbchk' needs to be clean? Unless there are reasons to the contrary, yes. > In any event, I would certainly appreciate not having to go > back and edit these dates... OK. It should be doable with a script, but "too hard to do compared to severity and uncertainty of issue" isn't a completely unexpected answer. > > If this bit were done right, I *think* it's actually pppd that would > > have a mapfile rather than this component. (Mapfiles were > > introduced well after pppd was integrated, and this was retrofitted > > on top. I'm not sure it's right.) Maybe filing a bug saying that > > the pppd plugins mapfiles smell funny would be a better answer than > > trying to track them. > > Without disagreeing about the specifics of pppd, I want to limit exclusions > as much as possible, and hit everything that even remotely approaches the > material in README.mapfiles. It would be good for anyone touching pppd to > read README.mapfiles, and the comment does no harm. OK. > > usr/src/cmd/csh/mapfile-intf > > [... et cetera ...] > > > > These are other slightly weird mapfiles. They don't seem to be > > versioning symbols that are defined by these objects, but rather > > working around some odd issues with interposers (and an unusual > > mech_krb5 design problem). I _guess_ tracking them is ok, though > > it's slightly surprising and may mean some needless build errors for > > those doing future maintenance in these areas. > > Or maybe 'interface'? That was my guess as to what 'intf' means. > > I largely agree, but think they are in the "close enough" > category. I'm not worried about the future maintenance, because > it's so easy to do, and because the odds are that you start a new > mapfile by making a copy of an existing one (almost all of which have > the comment). > > If experience proves me wrong, and this policy is deemed too noisy, we > can always dial up the exclusions later. OK; that makes sense. > > Maybe more importantly: the ARC manages the rules for public > > interfaces, not README.mapfiles or the tools community. > > I've changed it to: > > Scope reduction of Public interfaces is not allowed without > specific ARC review and approval. Thanks; that addresses it. > > usr/src/lib/libwrap/mapfile > > > > 3: what license would that be? > > There is a file named DISCLAIMER there that has a license, > and also a rather useless file named THIRDPARTYLICENSE.descrip. > > As my project doesn't actually have anything to do with licenses, > and as this isn't my change (it was approved and has integrated > previously), I'm going to leave it as is. > > On the other hand, if this is obviously wrong, and you know what > should be done to fix it, I'd be happy to put that in with my > putback --- let me know what to do. It just looked a bit strange to me. It's fine to leave it as-is. > > usr/src/tools/scripts/mapfilechk.1 > > > > 47-49: nit: I'd probably use .TP for this. > > This is carried from cddlchk.1. I'm not very deep with *roff, > but I copied the .TP use from one of our other manpages > (elfedit.1). and got rid of the .nf. I also did the > same for cddlchk.1. Looks nicer; thanks. > > 65-66: what "command-line help functions?" > > Also a carry-over from cddlchk.1. I have no idea what it means. > The code uses it for syntax errors. I've changed the text, > in both cddlchk.1 and mapfilechk.1 to be > > 2 Invalid command-line arguments were specified to the command. OK. > > usr/src/tools/scripts/mapfilechk.py > > > > 57-62: what handles the "syntax: glob" in your exception list? > > Nothing: Both 'hg mapfilechk' and the mapfilechk command line > interface will process any file that you explicitly point them at. > > The command line version of mapfilechk requires an explicit file > argument, so it never uses the exception list. > > However, when 'hg mapfilechk' is called without a file argument, > then it uses the active list for your workspace, filtered against > the exception list. That happens inside Cadmium (cdm.py). Ah, ok. Thanks. > > 76: nit: missing "return False" > > > > 104,107: 'if not' ... 'else' negative logic is hard to follow; try > > reversing. > > Both of these are existing code taken from cddlchk.py. Unless you feel > strongly, I'm going to leave them as they are. No, I don't feel strongly. > > usr/src/tools/scripts/nightly.sh > > > > 2997-2999: nit: suggest putting '|' at end of line. (So '\' isn't > > needed.) > > > > I see your point. Have you noticed all the places in nightly.sh that > do the same thing though? > > % grep '| \\$' nightly.sh | wc -l > 94 Yes; I've noticed. > I think I'll leave it like this and stick to the common style > of the existing script. Let me know if you disagree. I'd counter with: % egrep '\| *$' usr/src/tools/scripts/nightly.sh | wc -l 41 Yes, the "\" stuff is more common, but I don't think it's more attractive. To me, it's analogous to the C Style guide; we don't write: c = a + b; and instead write: c = a + b; But drive on if you prefer. nightly.sh already has a long history of mixed styles from many authors. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
