lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Uwe Schindler (JIRA)" <j...@apache.org>
Subject [jira] Commented: (LUCENE-2285) Code cleanup from all sorts of (trivial) warnings
Date Thu, 25 Feb 2010 18:32:27 GMT

    [ https://issues.apache.org/jira/browse/LUCENE-2285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838464#action_12838464
] 

Uwe Schindler commented on LUCENE-2285:
---------------------------------------

bq. The removed cast from TestCharArraySet is justified because you want to test the contains(Object)
method, which is exactly what happens. In fact, when I look at the code, I think there is
a wrong cast:

This is not true:
{noformat}
Index: src/test/org/apache/lucene/analysis/TestCharArrayMap.java
===================================================================
--- src/test/org/apache/lucene/analysis/TestCharArrayMap.java	(revision 916146)
+++ src/test/org/apache/lucene/analysis/TestCharArrayMap.java	(working copy)
@@ -76,7 +76,7 @@
     int n=0;
     for (Object o : cs) {
       assertTrue(cm.containsKey(o));
-      assertTrue(cm.containsKey((char[]) o));
+      assertTrue(cm.containsKey(o));
       n++;
     }
     assertEquals(hm.size(), n);
Index: src/test/org/apache/lucene/analysis/TestCharArraySet.java
===================================================================
--- src/test/org/apache/lucene/analysis/TestCharArraySet.java	(revision 916146)
+++ src/test/org/apache/lucene/analysis/TestCharArraySet.java	(working copy)
@@ -475,7 +475,7 @@
       assertFalse(CharArraySet.EMPTY_SET.contains(stopword));
     }
     assertFalse(CharArraySet.EMPTY_SET.contains((Object) "foo"));
-    assertFalse(CharArraySet.EMPTY_SET.contains((Object) "foo".toCharArray()));
+    assertFalse(CharArraySet.EMPTY_SET.contains("foo".toCharArray()));
     assertFalse(CharArraySet.EMPTY_SET.contains("foo".toCharArray(),0,3));
   }
{noformat}

The problem here is:
We have a char[] and a Object method. The check tests that also the Object method accepts
char[]. This is important if you cast CharArraySet to java.util.Set and call with char[].
So removin the cast for this test is simply wrong. You can check this with Clover. And you
patch even adds the same check two times - instead of forcing the right method.

And by the way: When I run "ant" and it compiles I get no warning message at all.

bq. Are you sure? I have another project which compiles w/ javac and it works fine. I'll check
it, but I trust you .

As I said before, java compiles need to simply ignore unknown SuppressWarnings (see Java Language
specs). It's only Eclipse that is to over

bq. About adding casts for clarity of code - I guess that's a matter of styling, but the cast
is truly unnecessary, and just produces a warning. I would like the code to be free of those,
but that's only my opinion.

Yes its a matter of styling, because of the same we don't want to have autoboxing in internal
lucene code, because autoboxing has a speed impact for some of Lucene's code (like collectors).
So we want to see what happens.

I want to understand that I apply a char -> int conversion, especially in our new TokenFilters
you can get a problem very fast as Character-methods are very sensitive if called with char
or int from the Unicode part. And I say it again, javac shows no warning, so I don't see any
need to change this, just because this Eclipse prints a useless warning. But you can switch
them off. I have some warnings i simply swicth off after creating a project with eclipse.
Like the problem with generified instanceof checks, which are a bug in Eclipse.

> Code cleanup from all sorts of (trivial) warnings
> -------------------------------------------------
>
>                 Key: LUCENE-2285
>                 URL: https://issues.apache.org/jira/browse/LUCENE-2285
>             Project: Lucene - Java
>          Issue Type: Improvement
>            Reporter: Shai Erera
>            Assignee: Uwe Schindler
>            Priority: Minor
>             Fix For: 3.1
>
>         Attachments: LUCENE-2285.patch
>
>
> I would like to do some code cleanup and remove all sorts of trivial warnings, like unnecessary
casts, problems w/ javadocs, unused variables, redundant null checks, unnecessary semicolon
etc. These are all very trivial and should not pose any problem.
> I'll create another issue for getting rid of deprecated code usage, like LuceneTestCase
and all sorts of deprecated constructors. That's also trivial because it only affects Lucene
code, but it's a different type of change.
> Another issue I'd like to create is about introducing more generics in the code, where
it's missing today - not changing existing API. There are many places in the code like that.
> So, with you permission, I'll start with the trivial ones first, and then move on to
the others.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org


Mime
View raw message