Robin Rosenberg escreveu:
I'm not fully convinced this is the right way after all. Good
performance is obviously good, but so is good encapsulation.
I've sometimes tried changing things like this even in pieces
of code that I really thought it should matter, but not been able
to measure any real improvemen even with performance measurment tools.
Obviously seeing that warning is annoying so maybe we should just set it to
ignore or exclude it from the project settings (if that is possible). The
only project where I think it might make a difference is the jgit part because
that is where we optimize and that is where I experimented with visibility
changes. In the Eclipse part we need to encapsulate more, partly because
Eclipse is less understood by the current authors than Java in general.
Encapsulation means encapsulating bad coding and bad design that comes
from lack of understanding of the framework we are working within.
Ok, so some more points for your consideration:
. I saw this /* private */ notation on eclipse code and found it
interesting.
. I won't bother measuaring any single case to make sure it is not
impacting performance under some circunstance, so resolving those
warnings puts me in the safe area. On the other hand, I think it is a
lot easier to tell if a patch is breaking encapsulation in a bad way
just by reviewing it, which is something that is already done.
Especially if it has the private modifier commented out. Someone can
even do a script to uncomment them and verify that it still builds
without errors.
. The ui part isn't supposed to be reused by other projects, so I think
encapsulation there is less important than for jgit. But even so, the
default modifier (or package-private) is good enough for encapsulation.
Other projects shouldn't write code in the the same packages from jgit,
if they do so they know that they are using internal things and they can
run into problems in the future.
That said, I'm ok with your patch too. I think the important is to
choose one so we stop mixing things, for consistency's sake.
[]s,
Roger.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
