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

Add retain to type export #109

Merged
merged 8 commits into from
May 13, 2024

Conversation

MandKastner
Copy link

No description provided.

Copy link

github-actions bot commented May 6, 2024

Test Results

    95 files  ±0      95 suites  ±0   38s ⏱️ -1s
27 181 tests ±0  27 181 ✅ ±0  0 💤 ±0  0 ❌ ±0 
27 182 runs  ±0  27 182 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 778001e. ± Comparison against base commit 067dd49.

♻️ This comment has been updated with latest results.

@@ -377,20 +379,35 @@ abstract class ForteFBTemplate<T extends FBType> extends ForteLibraryElementTemp

def protected generateSetInitialValuesDeclaration(Iterable<VarDeclaration> variables) '''
«IF !variables.empty»
void setInitialValues() override;
void setInitialValues(bool retain) override;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a new boolean parameter here? Is there a case where we do not want to retain variables marked as RETAIN?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we need the boolean argument. But it should be called cold. To indicate the right meaning of this variable.

if (!retain) {
«generateVariables(variables.filter[it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE) !== null && it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE).equals(RetainTag.RETAIN.string)])»
}
«generateVariables(variables.filter[it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE) === null || !it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE).equals(RetainTag.RETAIN.string)])»
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving the filter argument into a separate method.

«generateVariables(variables.filter[it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE) === null || !it.getAttributeValue(LibraryElementTags.RETAIN_ATTRIBUTE).equals(RetainTag.RETAIN.string)])»
'''

def private generateVariables(Iterable<VarDeclaration> variables)'''
Copy link
Member

Choose a reason for hiding this comment

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

Please give this method a more descriptive name, such as generateVariableDefaultAssignments.

«ENDIF»
'''

def protected generateSetInitialValuesDefinition(Iterable<VarDeclaration> variables) '''
«IF !variables.empty»
void «className»::setInitialValues(bool retain) {
void «className»::setInitialValues(bool cold) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that yesterday but can you please move the cold) { part into the if and else, and in the else only add ){

Because if there are no retain variables the cold param is not needed.

And as we are at it: In 4diac FORTE we have the coding guideline that function parameters stat with pa. So please change cold to paCold.

as discussed the setInitialValues() will only effect the values of variables during a warm restart. therefore it should only contain variables not tagged with RETAIN
«ENDIF»
'''


//todo escape new line character
Copy link
Member

Choose a reason for hiding this comment

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

Please fix (or remove) the todo.

def protected generateSetInitialValuesDefinition(Iterable<VarDeclaration> variables) '''
«IF(containsNonRetainedVariable(variables))»
void «className»::setInitialValues() {
«generateVariableDefaultAssignment(variables.filter[!isRetainedVariable(it)])»
}
«ENDIF»

Copy link
Member

Choose a reason for hiding this comment

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

Please move the empty line inside the ENDIF (where it was initially), so that it is only added when a setInitialValues is present.

@azoitl azoitl changed the base branch from develop to freeze May 13, 2024 13:58
@azoitl azoitl merged commit 12a7940 into eclipse-4diac:freeze May 13, 2024
4 checks passed
@MandKastner MandKastner deleted the AddRetainToTypeExport branch May 21, 2024 12:32
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.

None yet

3 participants