Summary: | [assist] void typed proposal may not be appropriate in many contexts | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Srikanth Sankaran <srikanth_sankaran> | ||||||
Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | david_audel | ||||||
Version: | 3.5 | Flags: | srikanth_sankaran:
review+
|
||||||
Target Milestone: | 3.6 M1 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Srikanth Sankaran
2009-04-06 06:32:08 EDT
See bug# 25876 & bug# 43897. We don't want to eliminate void typed proposals altogether, we may want to diminish their relevance so they are offered below others. Jay, can you take a look at this ? Normally we use the notion of expected type to boost the relavance of a proposal. In this case, we may want to diminish the relavance. Thanks. Consider this test: public class C { void foo00() {} void foo01(){} String foo0 () {return null;} float foo1 () {return 0;} int foo2() {return 0;} Long foo3() {return null;} long foo4() {return 0;} Integer foo5() {return 0;} void test() { foo| } } On Ctrl+Space, we get all foo methods get the same relevance value and ultimately listed alphabetically. My question is, if the completion node doesn't have an expected type, should we give higher relevance to void methods? In this case I would use the same relevance for all methods because it is not so rare to want to use a method without using the returned value (eg. List.add(Object)). Created attachment 135919 [details]
Proposed patch
The fix introduces a new relevance for methods returning void values with a value of -5. This way the relevance of such methods get reduced by 5. Test cases have been modified to relflect/test this behavior.
I looked the patch and i have several suggestions: - I do not understand why the void check is done in the for loop inside computeRelevanceForExpectingType(). I think it should be done at the beginning of the method with something like: private int computeRelevanceForExpectingType(TypeBinding proposalType){ if (proposalType == TypeBinding.VOID) return VOID; ... - Before this patch all the relevance constants were positive so there is no risk to compute a negative relevance. This patch change this rule and add a negative constant. To limit the risk of a negative relevance when adding a new constant in RelevanceConstants the new rules could be: - a relevance is the sum of constants defined in RelevanceConstants - each constant can be added only one time in the relevance of a proposal - the sum of all negative constants and R_DEFAULT must be positive You should add a comment in RelevanceConstants which explicitly describe these new rules. We should fix this bug for 3.6 as it is too late for 3.5. Agree with you on the changes, David. In fact, this change unearthed many more test cases that may have to be changed. And since this is going only for 3.6, I will work on fixing the test cases and submit a new patch after that. Created attachment 140340 [details]
Updated Patch
Attached the patch with the modifications as per suggestions and modified test cases.
Released Jay's fix in HEAD for 3.6M1 Verified for 3.6M1 (In reply to comment #10) > Verified for 3.6M1 > ... using I20090802-2000 build |