fix: shorter this access #1031
fix: shorter this access #1031
Conversation
monperrus
force-pushed
the
fix-this
branch
2 times, most recently
from
502a6aa
to
88d7d2d
Compare
| if (thisTarget.isImplicit()) { | ||
| // write nothing, "this" is implicit and we unfortunately cannot always know | ||
| // what the good target is in JDTTreeBuilder | ||
| return; |
There was a problem hiding this comment.
This return will produce a not compilable code because you always print a "." before the field access.
Will produce .field witch is not compilable
There was a problem hiding this comment.
no because the caller checks for implicitness as well (see ctVisitInvocation for instance).
however, this is an interesting question: who is responsible for printing the "."? probably the parent element, as done today (but consistently?).
however instead of
if (target.isImplicit()) {
scan(target);
print(".")
}
I would prefer
scan(target);
if (somethingHasBeenWritten()) {
print(".")
}
This would be less fragile.
(outside the scope of this PR but I'd like to have your opinion on this)
There was a problem hiding this comment.
No you remove the check: https://github.com/INRIA/spoon/pull/1031/files#diff-5f33bf5d1f38230de815e4c60a67a215L686
|
well done. then a test is missing, I'll add it.
|
monperrus
force-pushed
the
fix-this
branch
8 times, most recently
from
b3ada27
to
a81e380
Compare
monperrus
changed the title
| @@ -55,7 +56,8 @@ public void testQualifiedThisRef() { | |||
| ctTypes.add(ctClass); | |||
| printer.getElementPrinterHelper().writeHeader(ctTypes, imports); | |||
| printer.scan(ctClass); | |||
| assertTrue(printer.getResult().contains("Object o = QualifiedThisRef.Sub.this")); | |||
| Assert.assertTrue(printer.getResult().contains("Object o = this")); | |||
| Assert.assertTrue(printer.getResult().contains("Object o = QualifiedThisRef.this")); | |||
| scan(f.getTarget()); | ||
| if (!f.getTarget().isImplicit()) { | ||
| if (printer.hasNewContent()) { |
There was a problem hiding this comment.
Nice idea. But is it safe to store the snapshot in printer? What if scan will call printer.snapshotLength() too?
What about
int snapshot = printer.getSnapshotLength()
...
if(print.hasNewContent(snapshot)) {...}There was a problem hiding this comment.
you're right, replaced by a queue.
There was a problem hiding this comment.
what about this code?
printer.getSnapshotLength();
{
...some complex code with many ifs, loops
... and evil
return;
... added by somebody else after some years, will bring him hot time ...
}
if (printer.hasNewContent()) {...}this
int snapshot = printer.getSnapshotLength()
...
if(print.hasNewContent(snapshot)) {...}is little bit more code, but is really safe
There was a problem hiding this comment.
yes, but looks less elegant. let's wait for the devil to make ugly things :-).
|
I tried the code of this PR with my project and printed version compiles well. I also do not see problem in code ... but there are part which I do not understand, or I am lazy to understand ... :-) |
monperrus
force-pushed
the
fix-this
branch
2 times, most recently
from
02f17cb
to
b7f257a
Compare
monperrus
changed the title
monperrus
force-pushed
the
fix-this
branch
3 times, most recently
from
5e332ba
to
58658e4
Compare
|
I wanted to go first for shorter and more explicit this accesses. I only keep shorter for now because I don't want to break too much test cases from client code. For handling explicitness during refactoring, we have to remove implicit in the appropriate setters and refactoring methods. |
|
@tdurieux you can merge |
This is an important change in the way we print this access.
The current strategy of writing qualified this
v.l.pack.n.Foo.this.bar()yields unreadable code even in readable with-imports mode.This PR fixes this.
In the mid-term, I would like to completely get rid of checks on isImplicit for this accesses, but this requires some additional work on the model. As discussed recently, there are incorrect implicit targets, which nobody detects and reports because they are not printed. Only advanced transformers suffer from them.
The pretty-printing of this accesses was incredibly strongly specified (kudos to @GerardPaligot ), hence the number of changed tests, please review carefully.
closes #826