Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      there's a ton of compiler warning in the solr tree, and it's high time we cleaned them up, or annotate them to be suppressed so we can start making a bigger stink when/if code is added to the tree thta produces warnings (we'll never do a good job of noticing new warnings when we have ~175 existing ones)

      Using this issue to track related commits

      The goal of this issue should not be to change any functionality or APIs, just deal with each warning in the most appropriate way;

      • fix generic declarations
      • add SuppressWarning anotation if it's safe in context
      1. warning.cleanup.patch
        34 kB
        Hoss Man
      2. SOLR-2288_namedlist.patch
        12 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          Reminder to self: feedback from rmuir on the mailing list to replace the static EMPTY set/map refs w/type info that i added with direct usage like this...

          • this(fieldName, fieldType, analyzer, EMPTY_STRING_SET);
            + this(fieldName, fieldType, analyzer, Collections.<String>emptySet());
          Show
          Hoss Man added a comment - Reminder to self: feedback from rmuir on the mailing list to replace the static EMPTY set/map refs w/type info that i added with direct usage like this... this(fieldName, fieldType, analyzer, EMPTY_STRING_SET); + this(fieldName, fieldType, analyzer, Collections.<String>emptySet());
          Hide
          Hoss Man added a comment -

          Committed revision 1056612.

          Show
          Hoss Man added a comment - Committed revision 1056612.
          Hide
          Hoss Man added a comment -

          Committed revision 1056595.

          Show
          Hoss Man added a comment - Committed revision 1056595.
          Hide
          Hoss Man added a comment -

          Committed revision 1056594.

          Show
          Hoss Man added a comment - Committed revision 1056594.
          Hide
          Hoss Man added a comment -

          Committed revision 1056578.

          Show
          Hoss Man added a comment - Committed revision 1056578.
          Hide
          Hoss Man added a comment -

          In the absence of any direct object to the approach taken in my initial patch (i even solicited feedback from the generics policeman multiple times on IRC over the past few weeks) I went ahead and commited to trunk...

          Committed revision 1056558.

          as mentioned before, i'm going continue on with similar easy fixes around the tree w/o bothering to post patches to jira – we can use CTR if anyone has concerns about specific changes.

          eventually i'd like to try and port some of this to 3x, but that's a lower priority

          Show
          Hoss Man added a comment - In the absence of any direct object to the approach taken in my initial patch (i even solicited feedback from the generics policeman multiple times on IRC over the past few weeks) I went ahead and commited to trunk... Committed revision 1056558. as mentioned before, i'm going continue on with similar easy fixes around the tree w/o bothering to post patches to jira – we can use CTR if anyone has concerns about specific changes. eventually i'd like to try and port some of this to 3x, but that's a lower priority
          Hide
          Hoss Man added a comment -

          can we just use: NamedList<?> rather then bind it explicitly to Object?

          i think in the changes i made so far i used "?" every place i could. Binding to "?" is appropriate when you don't care what the typevar is, but "Object" is what you have to use when you know you are dealing with a collection of heterogenous objects – which is many uses of NamedList.

          in the cases where the code was dealing with a NamedList of homogeneous objects, i used the specific homogeneous type (ie: "NamedList<NamedList<String>>")

          That said: there certainly may be places where i could have used NamedList<?> and didn't realize it.

          Show
          Hoss Man added a comment - can we just use: NamedList<?> rather then bind it explicitly to Object? i think in the changes i made so far i used "?" every place i could. Binding to "?" is appropriate when you don't care what the typevar is, but "Object" is what you have to use when you know you are dealing with a collection of heterogenous objects – which is many uses of NamedList. in the cases where the code was dealing with a NamedList of homogeneous objects, i used the specific homogeneous type (ie: "NamedList<NamedList<String>>") That said: there certainly may be places where i could have used NamedList<?> and didn't realize it.
          Hide
          Robert Muir added a comment -

          Separately, i just want to say the following about NamedList:

          All uses of this API should really be reviewed. I'm quite aware that it warns you about the fact that its slow for certain operations,
          but in my opinion these slow operations such as get(String, int) should be deprecated and removed.

          Any users that are using NamedList in this way, especially in loops, are very likely using the wrong datastructure.

          Show
          Robert Muir added a comment - Separately, i just want to say the following about NamedList: All uses of this API should really be reviewed. I'm quite aware that it warns you about the fact that its slow for certain operations, but in my opinion these slow operations such as get(String, int) should be deprecated and removed. Any users that are using NamedList in this way, especially in loops, are very likely using the wrong datastructure.
          Hide
          Ryan McKinley added a comment -

          For compiler warnings... without chaning the API, can we just use: NamedList<?> rather then bind it explicitly to Object?

          Show
          Ryan McKinley added a comment - For compiler warnings... without chaning the API, can we just use: NamedList<?> rather then bind it explicitly to Object?
          Hide
          Hoss Man added a comment -

          just the implementation is different.

          fair enough – i ment i was trying to avoid changes to either the APIs or the internals, just focusing on the quick wins that were easy to review at a glance and shouldn't affect the bytecode (Collection<Object> instead of Collection; etc...)

          I don't expect that all compiler warnings can be dealt with using trivial patches, but that's what i was trying to focus on in this issue.

          changes to the internals of specific classes seem like they should be tracked in distinct issues with more visibility

          Show
          Hoss Man added a comment - just the implementation is different. fair enough – i ment i was trying to avoid changes to either the APIs or the internals, just focusing on the quick wins that were easy to review at a glance and shouldn't affect the bytecode (Collection<Object> instead of Collection; etc...) I don't expect that all compiler warnings can be dealt with using trivial patches, but that's what i was trying to focus on in this issue. changes to the internals of specific classes seem like they should be tracked in distinct issues with more visibility
          Hide
          Robert Muir added a comment -

          Robert: as mentioned, i'm trying to keep a narrow focus on this issue: dealing with warnings that can be cleaned up w/o changing functionality...

          Ok but i didnt change the functionality? the functionality is the same, just the implementation is different.

          This is the root cause of most of the compiler warnings, let's not dodge the issue.

          Show
          Robert Muir added a comment - Robert: as mentioned, i'm trying to keep a narrow focus on this issue: dealing with warnings that can be cleaned up w/o changing functionality... Ok but i didnt change the functionality? the functionality is the same, just the implementation is different. This is the root cause of most of the compiler warnings, let's not dodge the issue.
          Hide
          Hoss Man added a comment -

          Robert: as mentioned, i'm trying to keep a narrow focus on this issue: dealing with warnings that can be cleaned up w/o changing functionality...

          The goal of this issue should not be to change any functionality or APIs, just deal with each warning

          ...can we please confine discusions of changing the implementation of NamedList (or any other classes) to distinct issues? like SOLR-912?

          Show
          Hoss Man added a comment - Robert: as mentioned, i'm trying to keep a narrow focus on this issue: dealing with warnings that can be cleaned up w/o changing functionality... The goal of this issue should not be to change any functionality or APIs, just deal with each warning ...can we please confine discusions of changing the implementation of NamedList (or any other classes) to distinct issues? like SOLR-912 ?
          Hide
          Robert Muir added a comment -

          Hi Hoss Man, thanks for starting this issue.

          I looked at your patch, and personally I think NamedList should really be type-safe.
          If users want to use it in a type-unsafe way, thats fine, but the container itself shouldn't be List<Object>.

          Here's an initial patch (all tests pass)... it also removes the deprecated methods.

          Show
          Robert Muir added a comment - Hi Hoss Man, thanks for starting this issue. I looked at your patch, and personally I think NamedList should really be type-safe. If users want to use it in a type-unsafe way, thats fine, but the container itself shouldn't be List<Object>. Here's an initial patch (all tests pass)... it also removes the deprecated methods.
          Hide
          Hoss Man added a comment -

          here's a patch i worked up a bit today ... i started by trying to focus on the facet code, and then started bouncing around to areas where NamedList was being used w/o a bounds on the generic, but i know even with that i'm missing some.

          i'm posting this here in the hoops that the generics policeman (et. al.) will help give it a once over review – assuming the general approach taken here doesn't raise any red flags with anyone in the next day or so, i'll commit and continue on with similar easy fixes arround the tree w/o bothering to post patches to jira (since this type of patch tends to get stale quickly and leads to a lot of pain trying to keep up to date)

          Show
          Hoss Man added a comment - here's a patch i worked up a bit today ... i started by trying to focus on the facet code, and then started bouncing around to areas where NamedList was being used w/o a bounds on the generic, but i know even with that i'm missing some. i'm posting this here in the hoops that the generics policeman (et. al.) will help give it a once over review – assuming the general approach taken here doesn't raise any red flags with anyone in the next day or so, i'll commit and continue on with similar easy fixes arround the tree w/o bothering to post patches to jira (since this type of patch tends to get stale quickly and leads to a lot of pain trying to keep up to date)

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development