I've now applied the patch locally and done a detailed review. I'm
posting this a bit outside the context of recent discussions to simply
state my present opinion after looking into the patch.

Generally, this is a big improvement. So thanks, Glenn, for your work
here!

I'm also not particularly happy about the //CS* comments. To a certain
degree I think I could live with them. A count shows 279 usages. I think
that may be a tad too much. Maybe we can find something in between, like
making more use of the "error" severity. Most checks are just warnings
right now. So using errors will make it easier to enforce at least the
rules most important to us. I've also experimented with the regular
expressions:

    <module name="ConstantNameCheck">
      <property name="format" value="(^[A-Z](_?[A-Z0-9]+)*$)|log"/>
      <property name="severity" value="warning"/>
    </module>

This should already make several such //CS comments unnecessary. There
are other comments referencing ConstantNameCheck where we should rather
convert the name to upper case. That will cut down on these even further.
Like Chris suggested, we could then even decide to live with a few
warnings as long as we increase the severity of the most important rules
and set up a no-go policy for "errors".

I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java.
Glenn, was that an accidental overflow from your work on the new
features?

I have no problem with the sometimes rather generic Javadoc comments.
Every committer is invited to improve on those as he's passing over
particular code parts. I know that we have quite a bit of outdated
documentation already in our Javadocs. So these comments don't make the
situation worse IMO. The only thing we can do is gradually improve. But
at least the generic javadocs lets us cut down on the number of warnings
so we can really focus to improve there rather than capitulate before
thousands of warnings.

Finally a nit: some files have got method signatures with whitespace
before and after the parantheses. We don't traditionally do that but the
Checkstyle profile doesn't seem to catch that. I guess it would be safe
to add that rule so we can fix those occurences.


I would suggest the following as our next steps:

1. Clarify the thing with LineBreak*.
2. Decide (quickly, please) whether to remove the //CS comments or to
allow them for now and optionally do something about them later. (I'm
tending towards removing them but I don't have a problem if we do it the
other way.)
3. Commit the patch to Trunk more or less as is (pending //CS decision).
3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
before and after parantheses. Then remove "log"-related //CS constants
and excessive whitespace.
4. Merge the changes into the Temp_ComplexScripts parts.
5. Glenn could then provide a new patch against the branch which we
could do a cursory review on. We apply that and experiment with what
he's built. He can continue his work.
6. We continue to incrementally improve our coding standards.

I'm happy to do the grunt work. Like Glenn, I don't like to hold
principle discussions right now because that holds up several people
from doing day-to-day work. That doesn't mean we can't hold them, but I
don't see why we have to do it as a precondition to processing this
patch. The patch gets us further but doesn't preclude any futher
improvements later.

Please, let's get this done.


Jeremias Maerki

Reply via email to