Avoid private nonnested classes [LUCENE-3525]

[asfimport]

https://svn.apache.org/repos/asf/lucene/dev/trunk/lucene/src/java/org/apache/lucene/store/NativeFSLockFactory.java shows a private top-level class NativeFSLock. This is a JDK 1.0-ism which should be considered deprecated. Either use a nested class, or create a separate source file.

Can produce problems for tools. Cause of: https://netbeans.org/bugzilla/show_bug.cgi?id=204039

---

Migrated from LUCENE-3525 by Jesse Glick, updated Mar 25 2012 Attachments: LUCENE-3525.diff

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

How is this not a bug in Netbeans? :)

Seriously, this code compiles just fine, its legal java.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I agree with Robert: It's a bug with Netbeans.

But we should change those stupid side-by-side package private classes and make them subclasses. I have seen a lot of them in old code. Whenever I updated a class using this, I changed it.

[asfimport]

Jesse Glick (migrated from JIRA)

The bug in NetBeans was already given in the link above. But this construction is long obsolete and particularly tricky for tools to support in general (*), so it should be replaced whenever possible.

⭐ Since to find an associated source file you need to inspect a bytecode attribute. For JDK 1.1+ sources it suffices to just look at the class FQN to determine the source path.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Can you provide a patch for Lucene/Solr trunk, that transforms all those classes to static inner ones? This would be nice to have, but I see no real need for doing that now, so I am a little bit reluctant to do it.

[asfimport]

Jesse Glick (migrated from JIRA)

Patch to remove all private top-level classes from Lucene/Solr trunk sources including tests.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

This netbeans bug was just reported a few weeks ago - surely it's a transient bug and will be fixed soon?

We shouldn't blindly change package private classes to static inner clases - first of all, it sometimes makes no sense... why would NullUpdateLog be scoped within FSUpdateLong? Second, it changes the class names (i.e. breaks things Class.forName that are used by solr configuration).

[asfimport]

Jesse Glick (migrated from JIRA)

BTW I missed TransactionLog (in FSUpdateLog.java) before.

"surely it's a transient bug" - probably existed for years. There is just not that much code out there using JDK 1.0 private classes for people to notice it earlier.

"will be fixed soon" - no idea. For the same reason, it is not a high priority. Anyway there are surely other things affected. While hyperlinking stack traces with such classes is easy enough once you know about it (for at p.B(A.java:123) ignore B and open p/A.java), other tool features can be harder to fix since you need a bytecode parser.

"why would NullUpdateLog be scoped within FSUpdateLong" - no good reason that I can see. Why was it in FSUpdateLong.java? A candidate for refactoring of some kind, in this case probably to be moved into UpdateHandler.java.

"Class.forName used by Solr configuration" - perhaps, though remember that these classes are necessarily package-private so not usable via reflection unless setAccessible(true) is called. For such cases (is there a list?) you would want to move the class into its own source file. For a committer this would be simple enough; for purposes of a submitted patch I did not want to make any such changes because the diff would be unreviewable and unlikely to apply cleanly after other trunk changes.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I fixed that for DocSet and DocSetBase in Solr.