review2: fix: expression type cast source positions #2113
Conversation
pvojtechovsky
force-pushed
the
fixExpressionPosition
branch
2 times, most recently
from
08f4671
to
4f1f16f
Compare
pvojtechovsky
changed the title
| @@ -935,7 +936,10 @@ public boolean visit(BreakStatement breakStatement, BlockScope scope) { | |||
|
|
|||
| @Override | |||
| public boolean visit(CastExpression castExpression, BlockScope scope) { | |||
| context.casts.add(this.references.buildTypeReference(castExpression.type, scope)); | |||
| CastInfo ci = new CastInfo(); | |||
| ci.nrOfBrackets = ((castExpression.bits >>> 21) & 0xF); | |||
There was a problem hiding this comment.
ok this is a low level one ;) Could you explain the operation in a comment please?
|
|
||
| @Test | ||
| public void testExpressions() throws Exception { | ||
| //contract: the expression parameter declared like `String arg[]`, `String[] arg` and `String []arg` has correct positions |
There was a problem hiding this comment.
It seems to be a cut/paste contract not related to what you test
pvojtechovsky
force-pushed
the
fixExpressionPosition
branch
from
4f1f16f
to
aa32748
Compare
|
API changes: 1 (Detected by Revapi) Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180627.174231-177 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT
|
|
do we really to need a new interface? or would it be ok to only add a new implementation class? |
|
New interface is consistent with other SourcePosition based interfaces. And yes, I will write a code which will check whether source position implements |
|
Sometimes, it seems to me that we create too many interfaces, that have no essential semantic
meaning to be modeled, and that only an implementation class and proper polymorphism would be
sufficient.
And yes, I will write a code which will check whether source position implements
|CompoundSourcePosition|. It is not nice to ask if it implements |CompoundSourcePositionImpl|.
one option would be to add a method "isCompound" in SourcePosition.
|
|
I agree with you, but let's refactor all source position interfaces to keep whole SourcePosition model consistent.... but then it is a lot of changes :-} |
|
OK, so you would go with this one unmodified? |
yes, I prefer to keep one same pattern (iface + class) for all SourcePosition interfaces first. If we decide that this pattern is wrong in this case, then we should refactor it on all places where it occurs. I agree with this refactoring, but it should be different PR. |
|
rebase? |
pvojtechovsky
force-pushed
the
fixExpressionPosition
branch
from
aa32748
to
a28ff76
Compare
|
rebase done |
| @@ -935,7 +936,11 @@ public boolean visit(BreakStatement breakStatement, BlockScope scope) { | |||
|
|
|||
| @Override | |||
| public boolean visit(CastExpression castExpression, BlockScope scope) { | |||
| context.casts.add(this.references.buildTypeReference(castExpression.type, scope)); | |||
| CastInfo ci = new CastInfo(); | |||
| //the 8 bits from 21 to 28 represents number of enclosing brackets | |||
There was a problem hiding this comment.
Just for my information, because I'm curious ;) It's something you obtained by reverse engineering or there's a documentation somewhere with that info?
There was a problem hiding this comment.
There is a documentation of these bits in source code of compiler ... and I needed some luck to found it ;-)
|
LGTM |
fix of #2055