Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.x
    • Fix Version/s: 2.x
    • Component/s: Engine
    • Labels:
      None

      Description

      Add a configuration property which would change macro parameter passing from pass by name, to pass by value. I think in many instances this option will provide behavior that is more intuitive to the user. There are two important exceptions to this, references created by #define, and references created by BlockMacro's $bodyContent. This allows the user to still specify pass by name semantics when desired, for example:

      #define($x) #if($foo)$foo.bar#

      {else}None#end #end
      #go($x)

      In the above example '#if($foo)$foo.bar#{else}

      None#end' will be passed by name. In all other cases pass by value rules apply for example:

      #go($foo.bar [1, 2, 3] #if($bar)Yes#

      {else}

      No#end)

      In the above example all parameters will be evaluated to a value first, then passed to #go. This has potential performance improvements also.

      1. value.patch
        3 kB
        Byron Foster

        Activity

        Hide
        Nathan Bubna added a comment -

        -1 I'm not at all a fan of adding such a config property. It will add complexity to internal code that is already quite complex. It will add complexity to development and use of velocimacro libraries. It will add complexity to helping macro users on the mailing list, as it adds another setting to ask them about. It changes a very longstanding Velocity behavior. And for all that, it doesn't really added any needed new feature.

        I don't mind veering away from strict pass by name, as i have already said. But i do not think this sort of thing ought to be subject to configuration properties. I just don't think the additional complexity is worth it in this case.

        Show
        Nathan Bubna added a comment - -1 I'm not at all a fan of adding such a config property. It will add complexity to internal code that is already quite complex. It will add complexity to development and use of velocimacro libraries. It will add complexity to helping macro users on the mailing list, as it adds another setting to ask them about. It changes a very longstanding Velocity behavior. And for all that, it doesn't really added any needed new feature. I don't mind veering away from strict pass by name, as i have already said. But i do not think this sort of thing ought to be subject to configuration properties. I just don't think the additional complexity is worth it in this case.
        Hide
        Byron Foster added a comment -

        <qoute>
        it will add complexity to internal code that is already quite complex
        </qoute>

        I've attached the changes as a patch so the complexity can be assessed. I believe them to be slight. The pass by value logic is simpler compared to the code required to support pass by name.

        <qoute>
        It will add complexity to development and use of velocimacro libraries
        </qoute>

        I don't see this. The main intent of this approach is to provide a more predictable and intuitive behavior to macros. I think this follows a precedent set by the language world which has already travelled this road long before Velocity. I suspect that most developers who write #do($foo.bar) are thinking pass by value, rather then what is really going on. However, people who are more comfortable with pass-by-name can simply not flip this switch.

        <qoute>
        it will add complexity to helping macro users on the mailing list, as it adds another setting to ask them about.
        </qoute>

        That is possible.

        <qoute>
        It changes a very longstanding Velocity behavior
        </qoute>

        As a configuration switch, yes. However, I don't believe that because Velocity has done something for a along time makes it right, wrong or otherwise. I suspect that this behavior was simply carried over from the early days, rather then a carefully considered design criteria.

        <qoute>
        it doesn't really added any needed new feature.
        </qoute>

        I think it adds an easier to understand parameter passing model. This may be subjective, but again I point to the trend set by the language world. There are parameter passing use cases that now become more useful (such as #foo([$a, $b, $c]) and that are not laden with hidden gotchas. There are potential performance increases, for example the benchmark in the experimental directory runs 5-10% faster.

        I would argue that for velocity 2.0 this should be the default behavior. Adding this as a configuration parameter provides an opportunity to try this out without committing.

        Show
        Byron Foster added a comment - <qoute> it will add complexity to internal code that is already quite complex </qoute> I've attached the changes as a patch so the complexity can be assessed. I believe them to be slight. The pass by value logic is simpler compared to the code required to support pass by name. <qoute> It will add complexity to development and use of velocimacro libraries </qoute> I don't see this. The main intent of this approach is to provide a more predictable and intuitive behavior to macros. I think this follows a precedent set by the language world which has already travelled this road long before Velocity. I suspect that most developers who write #do($foo.bar) are thinking pass by value, rather then what is really going on. However, people who are more comfortable with pass-by-name can simply not flip this switch. <qoute> it will add complexity to helping macro users on the mailing list, as it adds another setting to ask them about. </qoute> That is possible. <qoute> It changes a very longstanding Velocity behavior </qoute> As a configuration switch, yes. However, I don't believe that because Velocity has done something for a along time makes it right, wrong or otherwise. I suspect that this behavior was simply carried over from the early days, rather then a carefully considered design criteria. <qoute> it doesn't really added any needed new feature. </qoute> I think it adds an easier to understand parameter passing model. This may be subjective, but again I point to the trend set by the language world. There are parameter passing use cases that now become more useful (such as #foo( [$a, $b, $c] ) and that are not laden with hidden gotchas. There are potential performance increases, for example the benchmark in the experimental directory runs 5-10% faster. I would argue that for velocity 2.0 this should be the default behavior. Adding this as a configuration parameter provides an opportunity to try this out without committing.
        Hide
        Nathan Bubna added a comment -

        If people have a switch to flip for this, then when creating macro libraries you have to consider whether the parameters are being passed by value or by name. This will require those sharing libraries with others (or their future selves) to document clearly what is expected of params, or design the macros so that it doesn't matter (if possible). Definitely more complicated.

        And saying it is a changes longstanding behavior doesn't mean that i'm saying it is right the way it is.

        As for performance, i'm far more concerned about my time than runtime. Velocity is not a bottleneck for me in my apps. Complicating user support, macro library development, and testing are not happy ideas for me. I'm developing an allergy to behavior-changing configuration switches and the poor cost/benefit ratio they often seem to have. The fewer configurable behaviors, the better. Leave config for constants (like max depths and bodyContent keys), external interfaces (like resource loaders and loggers) and extension hooks (like user directives and uberspects).

        Again, i'm open to changing how parameters are passed, but not with a configuration switch. We should just decide how we want various things passed and stick with it. Yes, that is limited in 1.x by concerns about backwards compatibility, but those are the breaks. If you really want to change this, you could always kick off a 2.0 branch and just change it outright, no config switch.

        For 1.x, we may be able to get away with passing literals by value (e.g. #foo( [$a, $b]

        { 'test': $this }

        3 [1..$ten] 'string' ) ) as that's pretty darn intuitive and unlikely to mess many users up, but right now i think we should leave all references and interpolated strings as pass by name.

        Show
        Nathan Bubna added a comment - If people have a switch to flip for this, then when creating macro libraries you have to consider whether the parameters are being passed by value or by name. This will require those sharing libraries with others (or their future selves) to document clearly what is expected of params, or design the macros so that it doesn't matter (if possible). Definitely more complicated. And saying it is a changes longstanding behavior doesn't mean that i'm saying it is right the way it is. As for performance, i'm far more concerned about my time than runtime. Velocity is not a bottleneck for me in my apps. Complicating user support, macro library development, and testing are not happy ideas for me. I'm developing an allergy to behavior-changing configuration switches and the poor cost/benefit ratio they often seem to have. The fewer configurable behaviors, the better. Leave config for constants (like max depths and bodyContent keys), external interfaces (like resource loaders and loggers) and extension hooks (like user directives and uberspects). Again, i'm open to changing how parameters are passed, but not with a configuration switch . We should just decide how we want various things passed and stick with it. Yes, that is limited in 1.x by concerns about backwards compatibility, but those are the breaks. If you really want to change this, you could always kick off a 2.0 branch and just change it outright, no config switch. For 1.x, we may be able to get away with passing literals by value (e.g. #foo( [$a, $b] { 'test': $this } 3 [1..$ten] 'string' ) ) as that's pretty darn intuitive and unlikely to mess many users up, but right now i think we should leave all references and interpolated strings as pass by name.
        Hide
        Byron Foster added a comment -

        Ok, I would argue the viability of creating generic cross project macro libraries currently in Velocity given several factors, including what you outline above. But certainly a macro param behavior switch doesn't improve the situation.

        I think a 2.0 branch is a good solution.

        Show
        Byron Foster added a comment - Ok, I would argue the viability of creating generic cross project macro libraries currently in Velocity given several factors, including what you outline above. But certainly a macro param behavior switch doesn't improve the situation. I think a 2.0 branch is a good solution.
        Hide
        Nathan Bubna added a comment -

        Thank you for bearing with my paranoia on this.

        Show
        Nathan Bubna added a comment - Thank you for bearing with my paranoia on this.
        Hide
        Nathan Bubna added a comment -

        Seems to me that Byron already made the switch to pass-by-value for 2.0. I don't think there's anything left to do here, especially since i remain heartily opposed to having this sort of behavior be an "option".

        I'm happy how it is, pass-by-name for the remainder of 1.x and pass-by-value for 2.0.

        Show
        Nathan Bubna added a comment - Seems to me that Byron already made the switch to pass-by-value for 2.0. I don't think there's anything left to do here, especially since i remain heartily opposed to having this sort of behavior be an "option". I'm happy how it is, pass-by-name for the remainder of 1.x and pass-by-value for 2.0.

          People

          • Assignee:
            Unassigned
            Reporter:
            Byron Foster
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development