Details

    • Type: Sub-task Sub-task
    • Status: Reopened Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: 4.0
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      The attached patch deprecates org.codehaus.groovy.control.Phases and introduces a new enum org.codehaus.groovy.control.CompilePhase that lists the same phases (plus non-phases startup and finished). All uses of Phases were converted from ints to the enum.

      Public APIs that were converted to enums had their int variant kept and deprecated, and each thunks to the enum variant.

      Why enums? The biggest advantage I see is that we can add/move phases without having to require that all other code that ever read constants from Phases be re-compiled. Now that 1.6 is Java 5 based we can use enums. Look at the Joint compile stuff, classes are all loaded into the conversion phase without any good place to layer on any addition steps. We could add JOINT_STUB_PREPARATION (for the non-failing resolve step) and JOINT_STUB_GENERATION. w/o having to re-number or go to floats.

      It even passes the retro tests!

        Activity

        Hide
        blackdrag blackdrag added a comment -

        getPhase():int -> getPahse():CompilePhase looks like a breaking change. Then, shouldn't nextPhase(CompilePhase) be an operation on the enumeration? Next thing is, hat gotoPhase(int) calls gotoPhase(CompilePhase). If the int taking method is overwritten, but only the CompilePhase taking method is called, then the method changed by the user is not called and his logic is changed. I see this also as incompatible change then. More or less the same goes for ProgressCallBack#call. Well in the later case the possibility is quite low, but it is a possible problematic point.

        then a question... why didn't you use a for-each in Java5#addPhaseOperations(CompileUnit)? Well not so important.

        What is missing in this patch is making Phases itself deprecated.

        Then about adding phases... as long as the numbers are there in parallel, your text is speaking against adding phases, because even if it is an enum, the ordinal number will still change, thus the phase number will change, and the used integer will be of false value. So adding new phases without the disadvantages you mentioned would be possible only after Phases has been completely replaced by CompilePhases.

        Then another thing... it doesn't matter how much phases you have, there will always be an operations that does not fit a certain phase by name or that has not really a good place. So instead having phases by functionality we have phases by semantic and stub generation is surely a functionality, as it is not a required phase by the compiler. Also the position where we might want to have the stubs to kick might move, what do we do then? Change CompilePhases? You may have noticed, that each phase already contains a more than one phase operation without going to floats or such things.

        Also, what if we in the future use a compiler that will work without stubs and compiles the java code directly then instead? Then these two phases would be strange looking at last.

        So all in all I vote against this change to happen for 1.6. It is a breaking change, and it doesn't matter how much you try to duplicate the methods, just because they are there, does not mean they will be called like before, and methods with the wrong return type are surely a problem.

        No I think if this change should be done, then as a breaking change from the beginning and as part of other compiler changes that will break things. I tend to say this will not happen before 2.0, so it will take several months before this can happen, but at last we would have only one release with lots of breaking changes instead of several releases where the ABI in each of them breaks a little here and there...

        ah yes, a side note... in pure groovy code without the usage of types, this change could have been done without being a breaking change

        Show
        blackdrag blackdrag added a comment - getPhase():int -> getPahse():CompilePhase looks like a breaking change. Then, shouldn't nextPhase(CompilePhase) be an operation on the enumeration? Next thing is, hat gotoPhase(int) calls gotoPhase(CompilePhase). If the int taking method is overwritten, but only the CompilePhase taking method is called, then the method changed by the user is not called and his logic is changed. I see this also as incompatible change then. More or less the same goes for ProgressCallBack#call. Well in the later case the possibility is quite low, but it is a possible problematic point. then a question... why didn't you use a for-each in Java5#addPhaseOperations(CompileUnit)? Well not so important. What is missing in this patch is making Phases itself deprecated. Then about adding phases... as long as the numbers are there in parallel, your text is speaking against adding phases, because even if it is an enum, the ordinal number will still change, thus the phase number will change, and the used integer will be of false value. So adding new phases without the disadvantages you mentioned would be possible only after Phases has been completely replaced by CompilePhases. Then another thing... it doesn't matter how much phases you have, there will always be an operations that does not fit a certain phase by name or that has not really a good place. So instead having phases by functionality we have phases by semantic and stub generation is surely a functionality, as it is not a required phase by the compiler. Also the position where we might want to have the stubs to kick might move, what do we do then? Change CompilePhases? You may have noticed, that each phase already contains a more than one phase operation without going to floats or such things. Also, what if we in the future use a compiler that will work without stubs and compiles the java code directly then instead? Then these two phases would be strange looking at last. So all in all I vote against this change to happen for 1.6. It is a breaking change, and it doesn't matter how much you try to duplicate the methods, just because they are there, does not mean they will be called like before, and methods with the wrong return type are surely a problem. No I think if this change should be done, then as a breaking change from the beginning and as part of other compiler changes that will break things. I tend to say this will not happen before 2.0, so it will take several months before this can happen, but at last we would have only one release with lots of breaking changes instead of several releases where the ABI in each of them breaks a little here and there... ah yes, a side note... in pure groovy code without the usage of types, this change could have been done without being a breaking change
        Guillaume Laforge made changes -
        Field Original Value New Value
        Fix Version/s 1.6-beta-2 [ 14261 ]
        Fix Version/s 1.6-beta-1 [ 14008 ]
        Guillaume Laforge made changes -
        Fix Version/s 1.6-rc-1 [ 14009 ]
        Fix Version/s 1.6-beta-2 [ 14261 ]
        Hide
        blackdrag blackdrag added a comment -

        Danno applied the patch I think

        Show
        blackdrag blackdrag added a comment - Danno applied the patch I think
        blackdrag blackdrag made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Hide
        Danno Ferrin added a comment -

        Nope, didn't apply the patch based on your feedback. The feedback seemed to me as an indication you, would rather not have it done. This seems more appropriate for Groovy 2.0 where we are breaking other stuff anyway.

        Show
        Danno Ferrin added a comment - Nope, didn't apply the patch based on your feedback. The feedback seemed to me as an indication you, would rather not have it done. This seems more appropriate for Groovy 2.0 where we are breaking other stuff anyway.
        Danno Ferrin made changes -
        Status Closed [ 6 ] Reopened [ 4 ]
        Assignee Jochen Theodorou [ blackdrag ]
        Resolution Fixed [ 1 ]
        Hide
        blackdrag blackdrag added a comment -

        but your enum is there, or not?

        Show
        blackdrag blackdrag added a comment - but your enum is there, or not?
        Hide
        Danno Ferrin added a comment -

        The enum is there. Integrating it into the compiler isn't. The compiler phases are still relying on magic constants, whereas the patch was to drive if off of enums instead of numbered constants.

        Show
        Danno Ferrin added a comment - The enum is there. Integrating it into the compiler isn't. The compiler phases are still relying on magic constants, whereas the patch was to drive if off of enums instead of numbered constants.
        Guillaume Laforge made changes -
        Fix Version/s 1.6-rc-1 [ 14009 ]
        Fix Version/s 1.7 [ 14014 ]
        Guillaume Laforge made changes -
        Parent GROOVY-3114 [ 75444 ]
        Issue Type Improvement [ 4 ] Sub-task [ 7 ]
        Guillaume Laforge made changes -
        Fix Version/s 1.7 [ 14014 ]
        blackdrag blackdrag made changes -
        Fix Version/s 2.0 [ 13489 ]
        blackdrag blackdrag made changes -
        Parent GROOVY-3114 [ 75444 ]
        Issue Type Sub-task [ 7 ] Bug [ 1 ]
        blackdrag blackdrag made changes -
        Parent GROOVY-4506 [ 117493 ]
        Issue Type Bug [ 1 ] Sub-task [ 7 ]
        Hide
        blackdrag blackdrag added a comment -

        I removed the fix version for this issue, since the new parent is now a Groovy 2.0 specific task and there is no need to let this issue appear independent of the parent task

        Show
        blackdrag blackdrag added a comment - I removed the fix version for this issue, since the new parent is now a Groovy 2.0 specific task and there is no need to let this issue appear independent of the parent task
        blackdrag blackdrag made changes -
        Fix Version/s 2.0 [ 13489 ]
        blackdrag blackdrag made changes -
        Fix Version/s 4.0 [ 18928 ]
        blackdrag blackdrag made changes -
        Component/s command line processing [ 10445 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Danno Ferrin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: