John Sonnenschein wrote:
> Hello
>
> could someone do a review of http://cr.opensolaris.org/~error404/140/
> for me
>
> That change passes all the tests /ws/scmtest/src/legacy/scm/tooltest
> can throw at it
usr/src/tools/onbld/Checks/Comments.py:
- I might be missing something here, but it seems like you're reversing
order of fields in the tuples that make up the list of cases:
137 com, id = case.split(' ')
138 cases.append( (com, id) )
...
143 for case in cases:
144 id, desc = case
- Why are you even splitting case in the loop on 143? You only use id
and desc to append to the nonexistent and nomatch error lists, whereas
above the code simply uses the case tuple (line 131-132, similarly for
the old code).
- It seems like line 146 should be followed by a "continue."
- For duplicate entries, you'll never get a match on line 156, right?
Using a single line as a basis for comparison, the old code checked each
duplicate instance, but the new code checks the aggregate of such
instances, and that seems wrong.
- Why test the entire comment first, and then trim to 40 chars if that
fails? Why not simply test the first 40 chars?
- In general, I'm not sure why you changed the structure of the "for
entered in insts:" loop. I know you need to change some of the details
to match the DbLookups.py changes, but the changes as they stand seem
less correct than the old code.
usr/src/tools/onbld/Checks/DbLookups.py:
245 if int(fields[0]):
246 continue
...really? Won't int() throw an exception if it gets a string that
doesn't represent an integer? What was wrong for testing for the string
"0"?
247 arc, case = arclist[data.index(line)]
...do you need to do this? Can't you just split fields[1]?
--Mark
