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

Fix nontrivial typos #2170

Merged
merged 4 commits into from
Jun 19, 2019
Merged

Fix nontrivial typos #2170

merged 4 commits into from
Jun 19, 2019

Conversation

beutlich
Copy link
Member

Preparation to fix #940.

Credits go to @kdavies4.

@beutlich beutlich added bug Critical/severe issue L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined L: Thermal.FluidHeatFlow Issue addresses Modelica.Thermal.FluidHeatFlow labels Feb 15, 2017
@beutlich beutlich self-assigned this Feb 15, 2017
@beutlich beutlich added enhancement New feature or enhancement and removed bug Critical/severe issue labels Feb 15, 2017
@beutlich beutlich added this to the MSL_next-MAJOR-version milestone Feb 15, 2017
@beutlich
Copy link
Member Author

@kristinmajetta Can you please check the changes for Spice3?

I noticed that mos2RenameParametersDev sets the obsolete variables m_drainPerimiter/m_sourcePerimiter. which looks like a real bug, not just a typo fix. It is changed to set m_drainPerimeter/m_sourcePerimeter. Is this fine for you?

@kristinmajetta
Copy link
Contributor

I noticed that mos2RenameParametersDev sets the obsolete variables m_drainPerimiter/m_sourcePerimiter. which looks like a real bug, not just a typo fix.

Yes, that was wrong. Resolved in master by 3ab3035 and maint/3.2.2 by 16f3f22.

@beutlich beutlich removed the request for review from kristinmajetta February 17, 2017 16:05
@kristinmajetta
Copy link
Contributor

Yes, setting the variables m_drainPerimeter/m_sourcePerimeter is fine for me.

@beutlich
Copy link
Member Author

beutlich commented Oct 4, 2017

Yes, setting the variables m_drainPerimeter/m_sourcePerimeter is fine for me.

Yes, and was already dealt with in February 2017.

@kristinmajetta
Copy link
Contributor

So? Can we close the ticket?

@beutlich
Copy link
Member Author

beutlich commented Oct 4, 2017

No. This ticket is on the non-trivial typos (e.g., jucntioncap, united_converging_crossection and lamda) in Spice3, Fluid.Dissipation and Thermal.FluidHeatFlow and schedulded for the next major release. The Spice3 issue on m_drainPerimiter/m_sourcePerimiter was detected (and resolved) en passant.

@beutlich beutlich mentioned this pull request Oct 18, 2017
Closed
hubertus65
hubertus65 previously approved these changes Jan 21, 2018
View reviewed changes
@MartinOtter
Copy link
Member

This is a non-backwards compatible change. Was there a decision that the next major MSL version will allow non-backwards compatible changes (and therefore support of the conversion annotations introduced in Modelica 3.4 is needed in the tools)

@beutlich
Copy link
Member Author

This is a non-backwards compatible change.

For that reason, the milestone is set to MSL_next-MAJOR-version, which is defined as the next (not yet specified) major version which is non-backward compatible and (might) need a conversion script.

This PR should not be merged to master right now.

@beutlich beutlich removed the L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined label Jun 12, 2018
hubertus65
hubertus65 approved these changes Jun 13, 2018
View reviewed changes
Copy link
Member

@hubertus65 hubertus65 left a comment

Choose a reason for hiding this comment

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

Still looks good

dietmarw
dietmarw requested changes Jun 14, 2018
View reviewed changes
Copy link
Member

@dietmarw dietmarw left a comment

Choose a reason for hiding this comment

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

Just to make sure that this does not get merged too soon. It is still missing the conversionScript additions that should follow with the renames.

@beutlich
Copy link
Member Author

beutlich commented May 3, 2019 •
edited

@HansOlsson I am struggling on conversion scripts. Given a record where a component is removed (between two library releases), how can the record ctor call be converted. Currently, my conversion test model that runs in MSL v3.2.3, but no longer runs in MSL v4.0.0 is:

model Issue940 "Conversion test for #940"
  parameter Modelica.Electrical.Spice3.Internal.Mosfet.Mosfet r1 = Modelica.Electrical.Spice3.Internal.Mosfet.Mosfet(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, true);
  parameter Modelica.Electrical.Spice3.Internal.Mosfet.Mosfet r2 = Modelica.Electrical.Spice3.Internal.Mosfet.Mosfet(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, m_drainPerimiter=19, m_sourcePerimiter=20, m_uic=true);
  annotation(experiment(StopTime=1), Documentation(info="<html>
<p>
Conversion test for <a href=\"https://github.com/modelica/ModelicaStandardLibrary/issues/940\">#940</a>.
</p>
</html>"));
end Issue940;

@HansOlsson
Copy link
Contributor

@HansOlsson I am struggling on conversion scripts. Given a record where a component is removed (between two library releases), how can the record ctor call be converted.

Record constructor calls should be similar to function calls, it's just that we have to consider that every record is potentially also a function.

However, looking closely it seems the problem is a record constructor (or function call) where elements in the record (or function) are removed, is that correct?
That case wasn't considered for the design of the conversion scripts; and especially the positional argument case will take some thought.

@beutlich
Copy link
Member Author

However, looking closely it seems the problem is a record constructor (or function call) where elements in the record (or function) are removed, is that correct?

Yes, that is the case here. I believe it is rather impossible to convert record ctor calls for record classes where components have been removed or component types are changed.

I will simply add a convertMessage here. Since Mosfet.Mosfet is in Modelica.Electrical.Spice3.Internal and shall not be used by users, there is enough hope that no user is affected.

@beutlich beutlich added task General work that is not related to a bug or feature and removed enhancement New feature or enhancement labels Jun 19, 2019
HansOlsson
HansOlsson approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks good.

@beutlich beutlich merged commit 486f622 into modelica:master Jun 19, 2019
@beutlich beutlich deleted the issue-940 branch June 19, 2019 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dietmarw dietmarw dietmarw approved these changes

@HansOlsson HansOlsson HansOlsson approved these changes

@hubertus65 hubertus65 hubertus65 approved these changes

Assignees

@beutlich beutlich

Labels
L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation L: Thermal.FluidHeatFlow Issue addresses Modelica.Thermal.FluidHeatFlow task General work that is not related to a bug or feature
Projects
None yet
Milestone
MSL4.0.0
Development

Successfully merging this pull request may close these issues.

Corrections to typos
7 participants
@beutlich @kristinmajetta @MartinOtter @HansOlsson @dietmarw @hubertus65 @kdavies4