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

review: fix object is not an template parameter implicitly #1461

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

public class ObjectIsNotParamTemplate extends ExtensionTemplate {

	//this is normal field of type Object - it must not be considered as template parameter automatically
	Object o = "XXX";
	
	//the "o" in the method name must not be substituted
	void method() {}
	
	@Local
	public ObjectIsNotParamTemplate() {
	}
}

This template generated method with name methXXXd and it is of course wrong. This PR fixes this behavior.

May be the origin idea of the author was that each field whose type is subtype of TemplateParameter is automatically a template parameter. If you think that this contract is correct then implement it in another PR. I will have no time to continue on this PR in near future, so please merge it as it is - with fix only.

@pvojtechovsky pvojtechovsky changed the title WiP: fix object is not an template parameter implicitly review: fix object is not an template parameter implicitly Jul 2, 2017
@@ -727,64 +728,14 @@ public void substituteStringLiteral() throws Exception {
public void substituteSubString() throws Exception {
//contract: the substitution of substrings works on named elements and references too
Launcher spoon = new Launcher();
spoon.addTemplateResource(new FileSystemFile("./src/test/java/spoon/test/template/testclasses/SubStringTemplate.java"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you deleted all those lines in this test: the original contracts remain true, as SubstringTemplate contain a @Parameter, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not intentional! I probably based this PR on wrong branch. Old tests must remain. There is no reason to remove them. If you would like to have it fixed soon then fix it please somehow. I have only slow access to github web these days... so I can't do it.

@pvojtechovsky
Copy link
Collaborator Author

Yes! Now the changes are exactly like it was intended at the beginning. Thank You Simon!

It is ready for merge form my point of view. Just consider the original intention
A) TemplateParameter is subtype of param type - origin code
B) TemplateParameter equals param type - this PR
C) Param type is subtype of TemplateParameter - may be the origin intention.

B is better then A and is good for all my personal uses. But may be C makes sense too. WDYT?

@surli
Copy link
Collaborator

surli commented Jul 7, 2017

C) Param type is subtype of TemplateParameter - may be the origin intention.

IMHO it was the original intention, else why do most of the metamodel interfaces implements TemplateParameter interface?

If I understand well, with your change we now have to specifically type a parameter as TemplateParameter to let it considered as a parameter (or we can always annotate it with @Parameter). And with C, it means that if you put a field with any type of the metamodel which implements TemplateParameter it will be considered as a parameter, for example a CtBlock. Am I right?

So in version C, it means if I need in my template to have an element of the metamodel as a field of the template, for decoupling my code or whatever, I'll have to be careful on the name I'm using because it'll be considered as a parameter...
I'm not sure why the template had this behaviour, for me it makes a lot more sense to explicitly specify which are the parameter inside the template and to consider anything else, NOT as a parameter. Any historical hint on that @monperrus ?

@pvojtechovsky
Copy link
Collaborator Author

I think that spoon model elements extends TemplateParameter mainly because it should be possible to use their instance as value of such template parameter. But I am interested in Martin's answer too ;)

@surli
Copy link
Collaborator

surli commented Aug 1, 2017

Maybe we can merge this one and let @monperrus answers later on the historical history of template parameters, wdyt @pvojtechovsky ?

@pvojtechovsky
Copy link
Collaborator Author

I agree

@surli surli merged commit 0da20b5 into INRIA:master Aug 1, 2017
@pvojtechovsky pvojtechovsky deleted the fixObjectIsNotAnParam branch August 1, 2017 18:53
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.

2 participants