Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(position): CtCatch has no modifiers, they are in CatchVariable #2156

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

pvojtechovsky
Copy link
Collaborator

The modifiers were on two places before

  1. CtCatch - wrong
  2. CtCactchVariable - OK

So this PR removes them from CtCatch

Note: CtCatch would need a bettet source position structure. I actually I used BodyHolderSourcePosition, but it is may be not fitting well here. WDYT? Can we live with that or we make new structure for that?

@tdurieux
Copy link
Collaborator

tdurieux commented Jul 3, 2018

Which position information you would like to add to CtCatch?

@monperrus
Copy link
Collaborator

LGTM

@surli surli merged commit 9a45459 into INRIA:master Jul 3, 2018
@surli
Copy link
Collaborator

surli commented Jul 3, 2018

I merged this one to be integrated in 7.0.0: concerning BodyHolderSourcePosition I let you discuss with @tdurieux and if something needs to be added it will be integrated in 7.1.0

@pvojtechovsky
Copy link
Collaborator Author

Which position information you would like to add to CtCatch?

catch has two child roles: parameter and body. So in ideal situation I need a source position with three pairs of start/end

  1. whole catch element
  2. parameter related part
  3. body related part

But may be we might be able to detect these child fragments from positions of children. In that case the catch might be represented by simple SourcePositionImpl with one start/end pair, which represents whole catch element.

WDYT?

We can improve/change it later

@pvojtechovsky pvojtechovsky deleted the fixCtCatchPosNoModifiers branch July 3, 2018 19:55
@tdurieux
Copy link
Collaborator

tdurieux commented Jul 4, 2018

  1. whole catch element
  2. parameter related part
  3. body related part

I think that these three elements will be useful for the sniper mode.
I think it is better to have those positions.
Do you think we should create a ParameterHolderSourcePosition that will be used for the catch and the try with resources?

@surli surli mentioned this pull request Jul 4, 2018
@pvojtechovsky
Copy link
Collaborator Author

I think that these three elements will be useful for the sniper mode.

I don't know. May be. Let's play with sniper mode code for some time and gather some experience and then we will see what we really need. Note sniper mode uses SourceFragments which are based on information from SourcePosition. And SourceFragments might probably easily extract some source positions from child elements too.

But may be we should collect the list of Spoon elements whose source position is not fitting well to current SourcePosition model. I see these:

  • try with resource
  • catch
  • switch and case and default
  • ... ?
    Then we will see what SourcePosition structures are missing ... if any

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants