tjolsen / ExpressionTemplates.jl

Expression Template framework for Julia
Other
3 stars 1 forks source link

code style improvements #1

Closed vtjnash closed 8 years ago

vtjnash commented 8 years ago

these generated functions are unnecessary: https://github.com/tjolsen/ExpressionTemplates.jl/blob/master/src/ExpressionTemplates.jl#L64-L70 the functions would be identical (but more efficient for the compiler to work with), if they were not

the macro are being used incorrectly: https://github.com/tjolsen/ExpressionTemplates.jl/blob/master/src/v_funcs.jl#L28 https://github.com/tjolsen/ExpressionTemplates.jl/blob/master/src/vs_ops.jl https://github.com/tjolsen/ExpressionTemplates.jl/blob/master/src/vv_ops.jl#L12 a macro should never have side-effects, such as eval. the macro in this case is simply unnecessary: the code inside the macro can be directly executed at the toplevel

tjolsen commented 8 years ago

Thanks for taking a look at my project. These were a few areas that gave me some trouble figuring out how to implement nicely, but I think what I settled on is a reasonable solution.

Regarding the generated functions, the main issue here is that macros are expanded before types are inferred. Thus, it is impossible to know whether any symbol refers to an array or not until after the macro expansion. The generated functions allow me to sidestep this issue by effectively deferring some of the pre-processing until after the types have been inferred. The generated functions are expanded before the code is compiled, so the compiler is never actually seeing them; it only sees the result of them. See the documentation for an explanation of this behavior, since it is critical to the functioning of the @et macro. http://docs.julialang.org/en/release-0.4/manual/metaprogramming/#generated-functions

Regarding the macros with side effects, these macros are simply there to save me the headache of writing a tremendous amount of boilerplate code and to make future extension of the module an easy task. The style and the functionality is pulled almost directly from the metaprogramming documentation (under code generation), so I'm forced to believe that it is, in some respects, idiomatic Julia. To be more specific, the ability to use symbol (expression?) interpolation ended up being a real time saver, since I just needed to supply a list of functions (written as expressions), and the macro expands it out to generate lots of boilerplate code relating to it. The code is then evaluated at top level when the module is compiled. http://docs.julialang.org/en/release-0.4/manual/metaprogramming/#code-generation

The use of generated functions felt like a bit of a hack when I put this together, but I was on a bit of a deadline for a class presentation at the time. If you have any suggestions on how to make this work, I'd be more than happy to consider it as a modification to this package. My only requirements are that it meet at least the performance standards (time + memory allocations) that I've come to expect from the existing package.

vtjnash commented 8 years ago

a macro with side-effects is a misunderstanding of the purpose of a macro. the @et macro is correct: it returns an expression that gets spliced into the function. the other macros i've pointed out should just be regular functions (or just evaluated directly, since you don't actually need them to be callable later), since that is how you are using them. e.g. a more idiomatic form would be:

for (foo, bar) in options
    @eval complex_expression_with($foo, $bar)
end

the alternative to your usage of generated functions here is not macros, it is simply a regular function. in particular, the following two expressions are exactly equivalent in behavior, but the use of @generated in the first one is non-idiomatic:

@generated f(args...) = :(expr(args...))
f(args...) = expr(args...)

the compiler sees both of these, but it will be more efficient at reasoning about the second one. the reason for this is that dispatch is defined as occurring on the runtime types of the arguments, so it is incorrect to assert that the compiler will see something different based on the inferred type of the arguments. type inference & associated optimizations may affect speed, but should never affect the answer.

tjolsen commented 8 years ago

I see what you mean. I've changed the code generation functions as suggested (not pushed yet). I'm running my benchmark code with @inline functions rather than @generated functions. I'll post back here when it's complete. So far, though, it at least passes the tests I have written to ensure correctness.

tjolsen commented 8 years ago

It looks like all is well with the changes. Thanks for your advice. I'm going to mark this as closed. Commit f7eea5ffa87f9bc3ebc4df06e5f5cb90ed873031 contains the changes.