[Checkers] Checker Framework code review pre-comments
mernst at csail.mit.edu
Sun Jun 1 13:40:12 EDT 2008
Matt and Mahmood-
I have read the (voluminous) code that you sent me for the review. (I
didn't read NonNullFlow because the printouts didn't include Flow, which is
necessary in order to understand NonNullFlow.)
Overall, I was pretty happy with the amount of documentation in the code.
I could understand most parts of it. (There were some glaring
counterexamples, like the Javari checker that was missing large amounts of
essential documentation.) As a minor point, please don't add useless
comments that just repeat the name of the parameter and its type, or in
which the @returns clause is essentially identical to the method summary.
Comments should enlighten, not merely repeat.
I have a suggestion regarding how to make the helpful comments in the
checkers/visitors/factories even more helpful. Currently, they are
organized as HTML lists:
<li>the java.lang.Class class
Instead of using automatic numbering, number the clauses by hand:
4. the java.lang.Class class
5. enum classes
If there were multiple HTML lists in one Javadoc, number sequentially
rather than starting subsequent lists from 1 again.
Then, wherever in the class that a particular rule is implemented, add a
// Case 4. the java.lang.Class class
// Case 5. enum classes
This will make it much easier to understand the purpose of each part of the
code, and to see whether anything has been omitted from the documentation
or from the code.
I found several of the classes unnecessarily difficult to read because the
methods in the class were not organized in a logical fashion that made them
easy to understand. So I added the following text to
Order of procedures
When you add a new procedure to a file, don't just type it wherever your
cursor happens to be. Instead, place related methods together. It is often
helpful to put a block comment (e.g., starting with a row of 75 asterisks)
at the beginning of each group of related methods. Such block comments
divide the file into sections that are readily apparent to readers.
In general, put public methods before private ones in your files. Organize
your file with helper methods (whether public or private) after the main
entry points. This permits readers to read your code top-down, which is
more comprehensible: the purpose of each piece of code, and how it fits
into the whole, is obvious. A reader can forward-reference to just the
specification, not the whole implementation, of a helper method. (This
doesn't mean you necessarily have to write your in a code top-down order,
but do organize it that way for readers.)
I have a marked-up copy of the code that I will give you, but I won't be
able to scan it to PDF until Monday morning. Then, you can divvy up the
comments and work from a color printout. I strongly suggest against
working from an electronic copy, because you will miss some of the
comments. Perhaps not every suggested change is a good idea, but do please
examine them all. Then, for ones that aren't a good idea, you can explain
to me why. That will improve my understanding (either of this code or in
general), and we can decide whether some other change (possibly to the
documentation instead or in addition) is superior, or I was just confused.
This step of us discussing the suggested changes is quite valuable for all
sides, since it's part of the learning process that makes everyone
(reviewers and reviewees alike) better coders. I consider this extremely
important even if some of you won't be around for a long time, because
improving your skills is part of both your and my jobs at MIT.
As is usual for our code reviews, each reviewer (in this case, Jeff, Telmo,
and me) will have a set of specific questions that seem worth discussing in
person. Here is my list (in some cases, I've indicated what the issue is;
in others, I'll bring it up in person):
GraphAnnotationRelations: whole class must be rewritten from scratch.
Particular offenders are .wildcard and .addSubtype (which is buggy).
For annotations (AnnotationMirror) what is the difference between
canonical names and qualified names? Why are names used for equality
checking, rather than overriding the AnnotationMirror.equals method?
What is the contract of equality for AnnotationMirror?
BaseTypeChecker vs. SourceChecker: Should all of the framework
functionality (such as Annotated*) rely on BaseTypeChecker instead of
SourceChecker? From a few casts, it seems like that is the case. Our
framework wouldn't be so useful for checkers not built on SourceChecker
in that case. Or, alternately, some functionality could be moved from
BaseTypeChecker into SourceChecker. I would also like to see some
documentation in SourceChecker regarding when one would want to use it
without using BaseTypeChecker; I think that would clarify things for me.
The purpose of SourceChecker is not particularly clear now.
JavariAnnotatedTypeFactory: There is no documentation in this, which is
unacceptable. As a result, I can't make heads or tails of it.
I'll talk with you at the code review on Monday.
More information about the checkers