7:59 a.m.
Hi, I'll cut away the questions I don't need to ask more about.
2008/5/29 Emmanuel Bernard <emmanuel(a)hibernate.org>:
Hi Sanne
On May 28, 2008, at 14:01, Sanne Grinovero wrote:
[...]
> 1)is it ok in case of a "code revision" to have a
commit of >100 files
> for very trivial changes ( wrong formatting, spelling typos in comments,
> unused import removals);
> do I have to warn you before commits of this type?
> should I leave them all as-is?
If you do this massive work, you need to warn people in advance and do it in
a day. It essentially means people will have hard time to sync up their
current work while you do that.
If you cna't then, do it by small batches.
Yes I understand that, but no code changes are involved just some comments,
imports and "final" changes so I hope not to cause difficult conflicts, if any.
How should I proceed? My changes are ready for commit, I just wait
for everybody saying it's ok, so I can update first. I've seen Hardy
committed this morning;
Also if my working this way goes in the way of other developers,
I'll just revert them all.
> 5)static logger:
> some classes (e.g. org.hibernate.search.engine.QueryLoader) "forget"
> to declare the Logger as static, if this is intentional I would like to
> know
> more about it. May I fix them all if I find more?
> Is it because of hot-redeploy? In this case I would need to remove
> some statics;-)
http://www.slf4j.org/faq.html#declared_static
But for classes used a lot, I tend to keep the static modifier.
Nice I didn't now that; I changed the logger I used for configuration
parsing to non-static, looks much better.
Also I think DocumentBuilder could benefit from a non-static logger,
but it is (also) being used by one static method for a trace; I could
a) leave it static
b) have the method get it's own logger
c) remove the tracing
d) convert all using methods from static
I would prefer c), or d) but this may create a lot of changes.
> A)defensive copy on public API (FindBugs):
> FindBugs is complaining about
> "May expose internal representation by incorporating reference to
> mutable object"
> in serveral places.
> I suppose we can trust our own code, but for example in
> FullTextQuery setProjection(String... fields)
> it would be wise to make an own private copy of the data.
> Too much paranoia? Also it would have some performance cost.
What do you want to copy? Projection is essentially a copy already (unless
you reference THIS but in this case you want the mutable object). Otherwise,
the method parameters are immutable, so this is not a problem.
I guess this was not a very good example;) FindBugs is speaking about the Array,
a fool user may initialize the arguments as an array, give it to the
method, keeping
a reference to the array, to later (concurrently?) change the
references to other objects
or change positions, so having access to Searche's "internal state".
My questions is a bit more general, not specifically about this method but about
us ignoring this FindBugs warning, be aware of it; I suppose it would
be too much
paranoia to really change all the dangerous methods, just verifying it is
an issue you are aware of; rock solid libraries shouldn't allow it,
but it has performance
impact.
> B) there's a STATIC field in SharedReaderProvider:
> "static Field subReadersField"
> it is looking quite suspect, is that really meant to be static?
Yes it's on purpose, it's a pointer to a Field object, no need to pay the
lookup more than once.
Ah sorry I should have looked better, I thought a Lucene
Field, not reflection.
May I make it final, to enhance readability and to avoid null checks in code?
(initializing it with a static helper method)
> C)Workspace contains many maps to needed objects,
> all having the same key "DirectoryProvider"; also locks are
> mantained in an array; Woudn't it be easier to have a single map,
> returning a container object with all needed stuff;
> This object could be passed to all other methods instead of the
> dir.Provider and avoid many more gets on the maps.
> also some locking code could go "local" in this container
> class, I've some difficulty in tracking the lock sequences.
Go ahead (open a JIRA issue), make sure it does no slow things down - it
shouldn't from what I've seen)
You mean slow down of development time or Search's performance?
Sanne