Comment 1 Srikanth Sankaran 2013-02-22 00:29:34 EST

Anirban, thanks for following up.

Comment 2 ANIRBAN CHAKRABORTY 2013-03-05 00:52:39 EST

Created attachment 227915 [details]
Patch for 'default' suggestion for interfaces in 1.8

Patch for 'default' suggestion for interfaces in 1.8. Testcase is added to CompletionTest.

Comment 3 Srikanth Sankaran 2013-03-05 07:14:17 EST

Patch looks heading in the right direction: Here are some review comments:

(1) As was pointed out in https://bugs.eclipse.org/bugs/show_bug.cgi?id=399770#c7, all files modified for Java 8 BETA branch should have the 
JCP disclaimer as part of the copyright - Both the modified files are
missing them.

(2) We should not propose default as a choice in the following location.
We do presently:

@interface A {  // @interface == annotation type declaration.
	de|
}

(3) testBug401487 sets compliance to 1.7 - why ? This is likely to be harmless
in this test, but it is needless and as such confusing. It could be set to 1.8 ?

(4) Please add more tests that show what happens when you complete inside
similar location in a class, annotation type, enumerations and inner interface.
Only in the last case we would expect default keyword to be proposed.

(5) Why is the instanceof check astNode instanceof CompletionOnFieldType 
required in completionOnFieldType ?

(6) findKeywordsForMember:

(a) remove the comment: "// checking the following with null checks at every step :
		// if((astNode != null) && ((CompletionOnFieldType) astNode).binding.declaringClass.isInterface() )" - not useful.

(b) does this code block:

if((this.compilerOptions.sourceLevel >= ClassFileConstants.JDK1_8) &&
  (astNode != null) && 
   ((astNodeBinding = ((CompletionOnFieldType) astNode).binding) != null) && 
   ((declaringClass = astNodeBinding.declaringClass) != null) &&
   declaringClass.isInterface() ) {
	keywords[count++] = Keywords.DEFAULT;
} 

really need those half a dozen extra pairs of () ? :)

Also is it better written as:

if (astNode instanceof CompletionOnFieldType && 
               this.compilerOptions.sourceLevel >= ClassFileConstants.JDK1_8) {
	FieldBinding astNodeBinding = ((CompletionOnFieldType) astNode).binding;
	ReferenceBinding declaringClass = astNodeBinding != null ? 
                                           astNodeBinding.declaringClass : null;
	if (declaringClass != null && declaringClass.isInterface())
	    keywords[count++] = Keywords.DEFAULT;
}


This latter code block (a) pulls up the most important check first (b)
pushes the locals into a new block and removes them from outer scope where
they are irrelevant. (c) Guards the cast with an instanceof check rendering
it absolutely safe: In future if some other client calls this method with
a different ast node type, this code will not cause a ClassCastException.

(A point worth noting is that astNode instanceof CompletionOnFieldType
automatically ensures that astNode is not null. So no need to check for
astNode != null.)

Pleaase incorporate these suggestions, run the tests and post a fresh patch,
TIA.

Comment 4 Srikanth Sankaran 2013-03-05 07:17:45 EST

(In reply to comment #3)

> Pleaase incorporate these suggestions, run the tests and post a fresh patch,
> TIA.

Looking at how localized the changes are it should be enough to rerun just
completion tests, Thanks Anirban.

Comment 5 ANIRBAN CHAKRABORTY 2013-03-11 02:57:17 EDT

Created attachment 228175 [details]
Patch after accommodating the review comments

Hello,
Attaching applicable, completion test clean patch which incorporates the review comments.
Thanks
Anirban

Comment 6 Srikanth Sankaran 2013-03-11 07:12:35 EDT

Fix and tests released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=7dd940af8bfbaf46f5a69ec7b85437ba951f7d8e

Thanks Anirban,