Community
Participate
Working Groups
On extracting a default method in an interface [WIP, bug 406786], the new extracted default method is not formatted. For example, selecting "return 0;" and extracting it to a method: interface A_test1 { default int foo() { return 0; // extract to method } } Results in: interface A_test1 { default int foo() { return extracted(); // extract to method } default int extracted(){return 0;} } ------------------------------------------- However, standalone formatting (Ctrl+Shift+F) of the above created default method works properly. Select "default int extracted(){return 0;}". Pressing Ctrl+Shift+F, results in: default int extracted() { return 0; } ------------------------------------------ On debugging, it was found that the 'kind' passed to DefaultCodeFormatter.format(int kind, String source, IRegion[] regions, int indentationLevel, String lineSeparator) is K_COMPILATION_UNIT in stand-alone case (via org.eclipse.jdt.internal.ui.text.java.JavaFormattingStrategy.format()). And, the 'kind' is K_CLASS_BODY_DECLARATIONS (for MethodDeclaration via ASTRewriteFormatter.formatNode(ASTNode node, String str, int indentationLevel)) where it does not work. In the latter case, in DefaultCodeFormatter#formatClassBodyDeclarations(..), parseClassBodyDeclarations(..) returns null. So it could be that parser encounters some error and hence further formatting is not done.
Load balancing, Thanks Anirban for taking a look.
Created attachment 235481 [details] patch for the fix patch for the fix
Comment on attachment 235481 [details] patch for the fix Please mark patch as patch, TIA.
(In reply to ANIRBAN CHAKRABORTY from comment #2) > Created attachment 235481 [details] > patch for the fix > > patch for the fix Anirban, this fix is bad. It is assuming that default will be the first keyword in the portion of the code being formatted. I don't think we should devise fixes assuming such things. An interface method could have the public keyword modifying it even though it is redundant. You need to understand why the formatting fails and fix that.
(In reply to Srikanth Sankaran from comment #4) > (In reply to ANIRBAN CHAKRABORTY from comment #2) > > Created attachment 235481 [details] > > patch for the fix > > > > patch for the fix > > Anirban, this fix is bad. It is assuming that default will be the first > keyword in the portion of the code being formatted. I don't think we should > devise fixes assuming such things. An interface method could have the public > keyword modifying it even though it is redundant. > > You need to understand why the formatting fails and fix that. Hello, The real reason format function is failing, is because it internally calls the parser. The parser is not prepared to take a isolated default function, and find its way to the goal. ('default' function feeds to interface body,which feeds to interface. So, default function is accepted by parser only if it occurs in an interface). In the current scenario, only the function (the generated code) needs to be successfully parsed, which the parser is unprepared of. The ideal solution is to change the parser, which would be a high impact change. But, since the current flow represents a generated code, which has specific syntax, I provided this fix with low impact. But, the ideal solution is, definitely, to fix the parser. Thanks Anirban
(In reply to ANIRBAN CHAKRABORTY from comment #5) > The ideal solution is to change the parser, which > would be a high impact change. I think we can manage this so it is a well contained change. > But, since the current flow represents a generated code, which has specific > syntax, I provided this fix with low impact. But this makes untenable assumptions. So let us work on a more orthodox fix.
(In reply to Srikanth Sankaran from comment #6) > (In reply to ANIRBAN CHAKRABORTY from comment #5) > > > The ideal solution is to change the parser, which > > would be a high impact change. > > I think we can manage this so it is a well contained change. It looks like all we need to do at the grammar level is to get rid of the production MethodHeader and rename the production DefaultMethodHeader to be MethodHeader and then reject default in class methods ?
Created attachment 235790 [details] patch for the fix accommodating the review comments patch for the fix accommodating the review comments, which complies with http://www.eclipse.org/legal/CoO.php
Here are the comments on the latest patch: 1. ASTRewriteStatementsTest: eliminate new String ("...") and replace directly with "..." 2. Do we really need the \r characters ? 3. Parser: We can simply change consumeMethodDeclaration to take an additional boolean to indicate whether it is a default method. 4. default modifier is allowed only in an interface ==> Default methods are allowed only in interfaces. 5. CRP: changes here can be eliminated. As of now, CRP and its subtypes will allow default modifiers in classes - this is incorrect. We should set a boolean tolerateDefaultMethods on the parser in parseClassBodyDeclarations and reset it on exit and use this boolean in consumeMethodDeclaration to decide whether to complain or not.
Created attachment 235826 [details] patch for the fix accommodating the review comments patch for the fix accommodating the review comments, which complies with http://www.eclipse.org/legal/CoO.php
Created attachment 235827 [details] Revised patch - Fixed copyright in DocumentElementParser, AssistParser - Renamed Parser#tolerateDefaultMethods to be tolerateDefaultClassMethods - Fixed bad spelling of ProblemReporter.defaultModifierIllegalySpecified - Parser#consumeMethodDeclaration - changed the if to be: if (isDefaultMethod && !this.tolerateDefaultClassMethods) i.e changed the order of checks. - Restored the copyright in readableNames.props. There is a bug in the tool. We need to always "eye-ball" the patch before posting to make sure extraneous/ wrong changes are not introduced. - In Parser#parseClassBodyDeclarations, we should initialize tolerateDefaultClassMethods to true only if source level >= 1.8, not otherwise. - Messages.properties: Changed the message to say: "Default methods are allowed only in interfaces with source level 1.8 or greater." Jay, please take it forward. I am unable to run jikespg successfully: Since I made some changes that are untested, you will have to: - Fix the copyright in readableNames - Update any failing tests for the expected message. This should be really called for only test files modified by Anirban's patch since this is a new message. - Run some smoke tests and release. TIA.
(In reply to Srikanth Sankaran from comment #11) > Created attachment 235827 [details] > Revised patch committed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7ad857ed6f69048496d054a318e72f2cedf0eb3f
Fixed
(In reply to Manoj Palat from comment #12) > (In reply to Srikanth Sankaran from comment #11) > > Created attachment 235827 [details] > > Revised patch > committed via > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=7ad857ed6f69048496d054a318e72f2cedf0eb3f This commit does not show up under BETA_JAVA8 branch which still shows my commit on behalf of Shankha as the latest: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=e593164022433d17467e437ca5f1d1580fcf9c34 If you fact if you click on Manoj's commit, the drop down box shows "master" - It does not show up under master either.
(In reply to Srikanth Sankaran from comment #14) > This commit does not show up under BETA_JAVA8 branch which still shows > my commit on behalf of Shankha as the latest: There seems to be some delay. This link does bring up correct history: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/?h=BETA_JAVA8&qt=range&q=7ad857ed6f69048496d054a318e72f2cedf0eb3f Also note that this commit's parent is also properly attached to Shankha's commit.
(In reply to Jayaprakash Arthanareeswaran from comment #15) > (In reply to Srikanth Sankaran from comment #14) > > This commit does not show up under BETA_JAVA8 branch which still shows > > my commit on behalf of Shankha as the latest: > > There seems to be some delay. This link does bring up correct history: > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/ > ?h=BETA_JAVA8&qt=range&q=7ad857ed6f69048496d054a318e72f2cedf0eb3f > > Also note that this commit's parent is also properly attached to Shankha's > commit. Not so sure: My recent commit which shows up as the top entry in BETA_JAVA8 here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git with a commit id of http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=1ca61ba390ea54fcabd09223853d7a3d6816c249 also shows Shankha's fix as the parent.
The bug is fixed, but the fix seems to have vanished into thin air, reopening until there is clarity as to what happened. The history web page shows my commit made 5 minutes ago and another made 30 hours ago with no trace of Manoj's commit. That I was able to repeatedly pull with git telling me nothing to update and that git allowed me to push without fast forward concerns tells me that the branch itself is intact. Manoj, please follow up, Take any help you need from Jay, TIA.
The CGit web interface is not too smart. If the URL query contains h=BETA_JAVA8, then it just selects this entry in the dropdown, although the commit may not have anything to do with the given commit id. The link in comment 12 tells that the commit somehow landed on git.eclipse.org, but it's not in any branch (and therefore, Srikanth and I don't get it when we just use git pull).
(fix for comment #18) ... although the *commit* may not have anything to do with the given *branch*.
Thanks to Jay for helping out with the error. Commited & available at: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=bea430fec612168745d91278c4d6f4fccc5c2fd4
(In reply to Markus Keller from comment #18) > The CGit web interface is not too smart. If the URL query contains > h=BETA_JAVA8, then it just selects this entry in the dropdown, although the > commit may not have anything to do with the given commit id. > > The link in comment 12 tells that the commit somehow landed on > git.eclipse.org, but it's not in any branch (and therefore, Srikanth and I > don't get it when we just use git pull). Just for the records: the dangling commit is because Git push (first) transferred the local commit(s) but then failed updating the branch/refs due to the update hook, which expected a Signed-off-by in the commit message.
