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.

Sign up for GitHub

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

Jump to bottom

Just use "redeclare" instead of "redeclare final" #3649

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Just use "redeclare" instead of "redeclare final" #3649

merged 1 commit into from
Feb 8, 2021

Conversation

HansOlsson
Copy link
Contributor

Otherwise the class will be final which in some cases lead to problems.
modelica/ModelicaSpecification#2676

@HansOlsson HansOlsson added L: Electrical.Machines Issue addresses Modelica.Electrical.Machines L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) specification Issue (also) addresses the Modelica language specification labels Oct 12, 2020
@HansOlsson HansOlsson requested review from AHaumer and casella and removed request for casella and AHaumer October 12, 2020 07:25
AHaumer
AHaumer reviewed Oct 12, 2020
View reviewed changes
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

I understand but I'm not involved in Fluid, so maybe ask someone more famailiar with Fluid?

@HansOlsson HansOlsson removed the L: Electrical.Machines Issue addresses Modelica.Electrical.Machines label Oct 12, 2020
@HansOlsson
Copy link
Contributor Author

I understand but I'm not involved in Fluid, so maybe ask someone more famailiar with Fluid?

Sorry, I got confused, one issue involved the Machines-library in the Fluid-library, not the Electrical.Machines-library.

@beutlich beutlich requested a review from thorade October 12, 2020 16:50
@beutlich beutlich requested review from casella and MartinOtter and removed request for casella, henrikt-ma and MartinOtter November 16, 2020 21:15
@HansOlsson HansOlsson mentioned this pull request Nov 24, 2020
Merged
henrikt-ma
henrikt-ma reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

The changes look harmless to me, as they only remove the use of final when redeclaring classes.

However, I am not sure exactly what problem is being solved here. My guess is that this is really just a matter of removing some final that are believed to have been added for the wrong reason (namely, to make sure that the redeclaration isn't treated as redeclare replaceable). If my guess is wrong, however, I am unable to tell whether the proposed removal of final would have the desired effect, and it must be noted that the very meaning of a class being final is subject to ongoing discussion in #2719.

@beutlich beutlich removed the request for review from casella December 19, 2020 09:55
@HansOlsson
Copy link
Contributor Author

However, I am not sure exactly what problem is being solved here. My guess is that this is really just a matter of removing some final that are believed to have been added for the wrong reason (namely, to make sure that the redeclaration isn't treated as redeclare replaceable). If my guess is wrong, however, I am unable to tell whether the proposed removal of final would have the desired effect, and it must be noted that the very meaning of a class being final is subject to ongoing discussion in #2719.

You mean modelica/ModelicaSpecification#2719
However, that issue is now resolved and the impact on this use of final redeclare is that it goes from a bit weird with unclear consequences to bad and often making other models illegal.
Thus I will merge this PR soonish.

@beutlich
Copy link
Member

beutlich commented Feb 5, 2021

Off-topic: I'll care for a clean commit history w/o merge commits.

@HansOlsson @beutlich
Just use "redeclare" instead of "redeclare final"
2365358 
Otherwise the class will be final which in some cases could lead to problems.
modelica/ModelicaSpecification#2676
@beutlich
Copy link
Member

beutlich commented Feb 5, 2021

Off-topic: I'll care for a clean commit history w/o merge commits.

Done.

@beutlich beutlich added this to the MSL4.1.0 milestone Feb 5, 2021
@HansOlsson HansOlsson merged commit d01222b into modelica:master Feb 8, 2021
@HansOlsson HansOlsson deleted the RedeclareFinal branch February 8, 2021 11:48
@beutlich
Copy link
Member

beutlich commented Feb 8, 2021

Should this be back-ported to the maintenance branches?

@HansOlsson
Copy link
Contributor Author

Should this be back-ported to the maintenance branches?

Yes.

This was referenced Feb 15, 2021
Merged
Merged
@beutlich
Copy link
Member

Should this be back-ported to the maintenance branches?

Yes.

Done by #3751 and #3752 before it will get forgotten.

HansOlsson added a commit to HansOlsson/Modelica that referenced this pull request Feb 25, 2021
@HansOlsson
Do not use redeclare final here either.
11007fe 
Missed in modelica#3649
@HansOlsson HansOlsson mentioned this pull request Feb 25, 2021
Merged
beutlich pushed a commit that referenced this pull request Feb 26, 2021
@HansOlsson
Do not use redeclare final here either. (#3764)
bfc5dd0 
Missed in #3649
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AHaumer AHaumer AHaumer left review comments

@henrikt-ma henrikt-ma henrikt-ma left review comments

@thorade thorade thorade approved these changes

@MartinOtter MartinOtter MartinOtter approved these changes

Assignees

@HansOlsson HansOlsson

Labels
L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) specification Issue (also) addresses the Modelica language specification
Projects
None yet
Milestone
MSL4.1.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@HansOlsson @beutlich @thorade @AHaumer @MartinOtter @henrikt-ma