Please use the Apache issue tracking system for new NetBeans issues (https://issues.apache.org/jira/projects/NETBEANS0/issues) !!
Bug 159944 - Extra space when rewriting MethodInvocationTree
Extra space when rewriting MethodInvocationTree
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Source
6.x
All All
: P3 (vote)
: 6.x
Assigned To: Jan Lahoda
issues@java
: NETFIX
: 178207 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-09 22:41 UTC by tronicek
Modified: 2010-04-07 04:42 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
test cases, including a tough one with comments (6.01 KB, text/plain)
2009-05-17 01:28 UTC, fommil
Details
test case and proposed fix (7.21 KB, text/plain)
2009-05-18 22:50 UTC, fommil
Details

Note You need to log in before you can comment on or make changes to this bug.
Description tronicek 2009-03-09 22:41:23 UTC
I rewrite MethodInvocationTree as follows:

                MethodInvocationTree original = (MethodInvocationTree) node;
                List<? extends ExpressionTree> oldArgs = original.getArguments();
                List<ExpressionTree> newArgs = new ArrayList<ExpressionTree>();
                newArgs.add(oldArgs.get(1));
                newArgs.add(oldArgs.get(0));
                modified = make.MethodInvocation((List<ExpressionTree>) original.getTypeArguments(),
                        original.getMethodSelect(), newArgs);
                System.out.println("original: " + node);
                System.out.println("modified: " + modified);
                wc.rewrite(node, modified);

Input: 
    int plus(int x, int y) {
        return x + y;
    }

    void m2() {
        plus(1, plus(2, 3));
    }

Output:

    void m2() {
        plus( plus(3, 2), 1);
    }

That is, there is extra space after '('.

The log is correct:

original: plus(1, plus(2, 3))
modified: plus(plus(2, 3), 1)
original: plus(2, 3)
modified: plus(3, 2)
Comment 1 David Strupl 2009-03-31 16:07:38 UTC
Resolving all issues with milestone "future" as LATER. If you feel strongly that
it should be implemented please reopen and set the target milestone to "next".
Comment 2 tronicek 2009-04-01 07:59:36 UTC
I am working on a refactoring module and these bugs are crucial for it to work.
Comment 3 fommil 2009-05-17 00:50:44 UTC
I'd like to NetFIX [1] this bug. Is it possible? [1] http://wiki.netbeans.org/NetFIX
Comment 4 fommil 2009-05-17 00:59:33 UTC
We're going to have to be careful about this one... ensure that comments and annotations on the parameter are also moved. I'll submit a more comprehensive 
test case.
Comment 5 fommil 2009-05-17 01:28:13 UTC
Created attachment 82252 [details]
test cases, including a tough one with comments
Comment 6 fommil 2009-05-18 22:50:54 UTC
Created attachment 82354 [details]
test case and proposed fix
Comment 7 fommil 2009-05-18 22:54:54 UTC
@moonko you're going to have to decide if this is actually a bug or not! There is precedent in the existing test cases that the extra space should be there. In 
fact, if you accept the patches to the .pass files then there is no need to accept the standalone test159944 method test.

The proposed fix adds an extra variable tracking how many delete operations have been performed. Instead of looking to see if we are the first parameter in 
the old list, this patch means we check to see if we are the first parameter in the new list.

I note that diffParameterList doesn't appear to be comment friendly! I will submit a bug shortly on this.
Comment 8 Rastislav Komara 2009-05-19 10:19:39 UTC
I accept this bug as legitimate issue. The white space insertion is based on user defined formatting properties. If
there no space allowed, the any white space inserted is bug.
Comment 9 fommil 2009-05-19 10:47:51 UTC
OK, let me know if you want me to look at this again to explicitly query user preferences. Incidentally, I don't know how to set preferences in a test case... 
some advice please?

I find the code to deal with NOCHANGE in CasualDiff.diffParameterList to be overly complicated. Why can't we just do a simple

  copyTo(bounds[0], bounds[1]);

and be done with it? I don't understand all this backwards/forwards token nonsense... it is certainly not dealing with whitespace or comments correctly 
anyway.
Comment 10 fommil 2009-07-03 13:52:42 UTC
Update on status, please?
Comment 11 Jan Lahoda 2009-08-20 09:59:25 UTC
Reassigning all moonko's java/source bugs to myself.
Comment 12 Jiri Kovalsky 2009-10-14 13:44:27 UTC
Any update on this Honzo? Thanks!
Comment 13 Jan Lahoda 2009-12-21 05:42:42 UTC
*** Bug 178207 has been marked as a duplicate of this bug. ***
Comment 14 Jan Lahoda 2010-04-03 19:20:39 UTC
Thanks for the patch. I have pushed a slightly modified version of the patch:
http://hg.netbeans.org/jet-main/rev/43df26622ff0

I am looking into the comments-related problem.
Comment 15 Quality Engineering 2010-04-04 04:32:21 UTC
Integrated into 'main-golden', will be available in build *201004040201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/43df26622ff0
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #159944: when deleting leading method parameters, delete also trailing whitespaces after comma.
Contributed by Sam Halliday (fommil@netbeans.org).
Comment 16 Jan Lahoda 2010-04-06 14:11:21 UTC
The comments usecase should work (provided that the comments are correctly attached to the trees using GeneratorUtilities.importComments):
http://hg.netbeans.org/jet-main/rev/13418ff046d9
Comment 17 Quality Engineering 2010-04-07 04:42:43 UTC
Integrated into 'main-golden', will be available in build *201004070201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/13418ff046d9
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #159944: More correct handling of comments inside method invocations.
Partially contributed by Sam Halliday (fommil@netbeans.org).


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo