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

More concise way to render param types and values #136

Open
wookayin opened this issue Feb 8, 2014 · 9 comments
Open

More concise way to render param types and values #136

wookayin opened this issue Feb 8, 2014 · 9 comments

Comments

@wookayin
Copy link
Collaborator

wookayin commented Feb 8, 2014

The old way

For example, we have templates such as

${<if !e.Output.Param.Type.Array}
        __expected = ${e.Output}
${<else}
        __expected = (${foreach e.Output.ValueList v ,}
            ${v}${end}
        )
${<end}

or

${<if !in.Param.Type.Array}
            ${in.Param.Type.Primitive} ${in.Param.Name} = ${in};
${<else}
${<if !Options.cpp11}
            ${in.Param.Type.Primitive} ${in.Param.Name}[] = {${foreach in.ValueList v ,}
                ${v}${end}
            };
${<else}
            vector<${in.Param.Type.Primitive}> ${in.Param.Name} = {${foreach in.ValueList v ,}
                ${v}${end}
            };
${<end}
${<end}

Suggestion

A little messy, isn't it? Instead, I imagine a way that the parameter value is rendered directly in a polymorphic way: if it was a scalar, render it as-is, and if was a vector, render it in the way the compiler can interpret it.

We can know the language trait informations and the way how the parameter values should be rendered, given the context and the options.

The new way

Here are examples:

        __expected = ${e.Output}
  • Assuming the parameter is a primitive -- rendered as 1 or "YES" or 3.14159265358979 ...
  • Assuming the parameter is an array. Currently, ${e.Output} renders into the form of {1, 2, 3, 4, 5}. The modified render emits (1, 2, 3, 4, 5) -- the tuple, which is the way it should be written in python.

or

            ${in.Param.Type} ${in.Param.Name} = ${in};
  • Type becomes, for example, int, vector<int>, vector<long long> (in C++), or int[] (in Java), and so on...
  • ${in} (or in.Value ?) becomes 3, {1, 2, 3}, ... (no foreach blahblah)
    • Here comes a pitfall. Without C++11, we can't use initializer lists. But given the rendering context, we are provided whether C++11 feature is enabled or not in the configuration. So an alternative is required depending on this.
    • Surely, if we can't achieve it in a generic way, then we should branch the cases (as right now)
    • Or, introducing ${in.Declaration} -- writing the variable declaration code in the Java code, not in the template?

Remarks

  • This is just an idea for improvement - not a major nor pressing issue.
  • Considering various ways and cases, w.r.t various languages and parameter types (and grid-like options for String[] maybe), there are lots of things to be concerned.
  • Hope it helps us to maintain a much clean and readable template.
@wookayin
Copy link
Collaborator Author

I'm almost done with this feature (coming soon!) This is slightly related to #142.

zen0wu referenced this issue Mar 21, 2014
…ting input arrays

   * By using a modified version of jmte, with the ${<X} token
@zen0wu
Copy link
Owner

zen0wu commented May 6, 2014

I just found out a fundamental design error, which causes this issue (messy template) and also related to #142 and #151. That is, the ParamValue class, should not try to cover both array values and normal values. The right design should be two separate classes, ParamValue and ParamValueList, in which ParamValueList contains a list of ParamValue.

In this case, the language renderer should be able to render ParamValueList separately and get the correct format. User can choose between render the ParamValueList directly or foreach over its values and render each ParamValue.

Right now the code is wrong when try to convert to values from TC API model to greed model. In AbstractLangauge.java line 64, the renderParamValue method is called. However, no rendering should happen at this time. The data model should be language independent and the rendering is delayed to the code generation phase.

@vexorian @wookayin What do you guys think?

@wookayin
Copy link
Collaborator Author

wookayin commented May 6, 2014

@shivawu I totally agree with you. Actually, I had caught the deficit and was making a way to fix this.
Unfortunately I was too busy these days so I didn't complete the job. If you wish, I will share the working experimental branch which is in a temporary status.

As I previously mentioned in the other issues, the ParamValue (indeed it is Argument conceptually) should contain the value in the canonical form (i.e. independent of language). I think we might introduce a breaking API changes (2.0 is still in a tentative status!), which is expectedly to be accompanied by a little changes on templates (e.g. deprecating the usage of *.ValueList). I hope we could discuss on it after sharing the works.

@zen0wu
Copy link
Owner

zen0wu commented May 7, 2014

Yes, please share your working branch and I'll take a look, then we can find the best way to do this.

@zen0wu
Copy link
Owner

zen0wu commented May 7, 2014

What do you think of this commit?

It basically separate the ParamValue and ParamValueList, and the templates are still the same.
The new data template engine solves the problem of #142 and #151. But it's not fully tested, and simplifying the template is going to be a tough work.

This is only a proposal, if you have better solutions, that would be great.

@wookayin
Copy link
Collaborator Author

wookayin commented Jul 6, 2014

@shivawu Hello, we were maybe too busy these days. I finally managed to continue my previous refactoring and improvements on this issues: it's almost done. I am now sharing my branch, please feel free to look at it.

Technical details on the implementations you might be interested in can be found in the commit messages.

@zen0wu
Copy link
Owner

zen0wu commented Jul 22, 2014

@wookayin I've gone through your change of param value briefly, got a few questions.
So the argument is a container of type and value, in which the type can be a primitive or an array, and the ParamValue is changed to a Iterable, but what does the ParamValue stand for exactly?

Also, when I implement my branch (param-value), I found that when we start a rendering process, the language related engine is fixed, so when rendering the data template, there's no way for the engine to know that here we must use a bare engine instead of the language specific engine. So your way to solve this problem, is to distinguish the argument (which is a pure value), and the param value (which is language related), and use argument in the data template rather than the param value to render it to its canonical form, it this right?

@wookayin
Copy link
Collaborator Author

wookayin commented Sep 9, 2014

I am sorry I missed your previous comment. Here are my answers: as of current branch (up to commit cca51df),

  • Argument: The actual value passed to the method(function), each bound to a single parameter.
  • ParamValue: The value object itself, consisting the type and the value representation in a string (canonized)

Actually the names have to be exchanged (Argument <-> ParamValue), to fit to the our well-known definition. Some names (especially, ParamValue) are inappropriate.

I had already noticed this, but it seems to be introducing a backward-incompatibility on templates; so we might need some discussions. I remember that I once wrote a commit of exchanging those class names, but I see I didn't attached the commit into this branch. It looks that you agree with this, don't you?

@wookayin
Copy link
Collaborator Author

wookayin commented Sep 9, 2014

And yes, to distinguish the argument (currently ParamValue, but Argument afterwards) and the value (current Arguemnt -- wrong, but what appropriate name it will have?) themselves.

The value's are rendered into appropriate forms (e.g. 1LL in C++, 1L in Java) according to the way that the LanguageRenderer specifies.

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

No branches or pull requests

2 participants