Add date and time utilities #3247
Conversation
beutlich
added
enhancement
|
The general idea is good, but:
I tried to look at the Buildings-library. It uses full names in the records, but unfortunately it contains its own leap-year handling (hopefully correct): |
|
I started with an operator record for TimeType to also allow addition, subtraction etc. That meant, we also would need to consider a TimeDeltaType and conversion to absolute time, which is not straight-forward. Therefore, I rejected this first idea and went for the TimeType record as absolute point in time and without numerical conversion. (String conversion is still to do, but without handling of leap-seconds).
OK, will rename.
Hm, @GallLeo what do you think? @mwetter considers leap years only in range [2020, 2030] which might not be sufficient in a general purpose library. |
|
If you want to consider leap years, daylight savings, etc... You need to work on some absolute unit of s/etc relative from 1970... I think that's the only sane way of dealing with any kind of delta and ambiguity... Just convert between tm and time_t if you want to use that (and run the calculation in external C). |
|
Here is a first (and incomplete) draft for conversion of date/time to string. Not sure if it is good enough ❔ . Any feedback is appreciated. |
|
@HansOlsson @DagBruck @t-sommer @StephanZiegler Yesterday, I noticed by chance that there are Testing.Utilities.Time.{DateTime, Duration} available in Dymola as operator records. This is exactly the way I wanted to go here for MSL, too. I believe these utilities came up as necessary side-product when meaasuring time in Modelica. Now, that I am aware of this library, I am also biased with the further development. My question is, if you could think of contributing or sharing Modelica code for further development as OSS, such that I am sure, no copyright will be violated? |
|
We would be happy to move the operator records from the Testing library to the MSL. |
beutlich
force-pushed
the
add-time-utilities
branch
from
b201bac to
9575f16
Compare
|
|
|
@m-kessler Thank you for the generous offer. I rebased the commits of this PR on master and resolved the merge conficts. This PR branch is now ready to be used. I added you as contributor (with access role = Triage) such that you can directly push to this branch (hopefully). I already created a TimeType record. This can be now enhanced by using the two different operator records from the Testing package. It would be good if we can use the exsting names of the TimeType record components (which are different than the Testing package.) |
beutlich
force-pushed
the
add-time-utilities
branch
from
9575f16 to
362c49d
Compare
beutlich
changed the title
beutlich
force-pushed
the
add-time-utilities
branch
4 times, most recently
from
8cc2d55 to
47d29e8
Compare
Creating a Duration does not work anymore that way, due to "e547700 - Make Duration constructors unambiguous"
Co-Authored-By: Thomas Beutlich <modelica@tbeu.de>
... as the one we had did not catch negative durations with multiple days.
... as this is not supported at the moment.
... to fix inheritance, as its not allowed to extend from an operator record in Modelica 3.4
Conversion for MSL v4.0.0 no longer is suitable, hence migrate to next major version.
beutlich
force-pushed
the
add-time-utilities
branch
from
6c74cc5 to
a37706a
Compare
WalkthroughThis pull request introduces new time-handling functionalities across both external C sources and Modelica libraries. It adds implementation and tests for date and time formatting/parsing functions, integrates a new nearest-integer function in math blocks, and refines icon graphics. Build scripts, license files, conversion scripts, and test suites have been updated accordingly. Additionally, new packages and functions for time manipulation have been incorporated into the internal utilities, while deprecated implementations have been removed. Changes
Sequence Diagram(s)sequenceDiagram
participant TestApp
participant ModelicaTime_C
TestApp->>ModelicaTime_C: Initialize time components
TestApp->>ModelicaTime_C: Call ModelicaTime_strftime(...)
ModelicaTime_C-->>TestApp: Return formatted string
TestApp->>ModelicaTime_C: Call ModelicaTime_strptime(formatted string)
ModelicaTime_C-->>TestApp: Return parsed time components
sequenceDiagram
participant User
participant ModelicaTime_Internal
User->>ModelicaTime_Internal: Call getTime
ModelicaTime_Internal-->>User: Return DateTime record
User->>ModelicaTime_Internal: Call dayOfWeek(DateTime)
ModelicaTime_Internal-->>User: Return day of week
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Modelica/Resources/Scripts/Conversion/ConvertModelica_from_4.0.0_to_5.0.0.mos (1)
1-6: 💡 Verification agent🧩 Analysis chain
Function relocation handled, but what about return type changes?
The conversion script correctly handles the relocation of the getTime function from System to Internal.Time. However, according to the PR objectives, getTime now returns a record instead of its previous form.
Are additional conversions needed for code that uses the return value of getTime? This might require updates to existing code that expects the old return type.
🏁 Script executed:
#!/bin/bash # Search for usages of getTime in the codebase to assess potential impact echo "Searching for usages of Modelica.Utilities.System.getTime..." rg -l "Modelica\.Utilities\.System\.getTime" --glob "*.mo" echo "Checking if there are examples of how the return value is used..." rg "getTime\(\).*" --glob "*.mo"Length of output: 995
Action Required: Update Destructuring for New Return Type
The conversion script in Modelica/Resources/Scripts/Conversion/ConvertModelica_from_4.0.0_to_5.0.0.mos correctly relocates the
getTimefunction call fromModelica.Utilities.SystemtoModelica.Utilities.Internal.Time. However, as verified by the search results, several files (such as ModelicaTestConversion5.mo, ModelicaTest/Utilities.mo, Modelica/Utilities/Time.mo, and Modelica/Utilities/Internal.mo) still use destructuring assignments withgetTime()that expect multiple discrete values. With the updatedgetTimenow returning a record (e.g., a DateTime record), these usages are no longer compatible.Next Steps:
- Review and update destructuring assignments: Modify code that deconstructs the return value (e.g.,
(ms, sec, min, hour, day, mon, year) = getTime();) to extract the appropriate record fields.- Ensure downstream compatibility: Validate that any references to individual time components map correctly to the corresponding fields within the new record.
🧹 Nitpick comments (11)
.CI/Test/ModelicaTime.c (1)
7-29: Good test coverage for date/time formatting and parsing functions.The test verifies both the formatting of date/time values to a string and parsing a string back to date/time components with appropriate assertions to validate the results.
Two suggestions for improvement:
- Consider adding tests for edge cases (leap years, month boundaries, etc.)
- Consider testing with different format strings to ensure all format specifiers work correctly
ModelicaTest/Utilities.mo (1)
984-987: Integration intotestAll.Including the new tests (
Time,Duration,DateTime) ensures they run alongside the existing suite. Consider aggregating the boolean results (e.g., using logical AND) if you wish to fail fast upon any test’s failure. Otherwise, this is a welcome addition.Modelica/Resources/C-Sources/ModelicaTime.h (2)
55-76: Macros for Non-null Pointers
The approach to annotate function parameters with nonnull attributes is effective. However, please ensure that all compilers and build environments you target support these macros. Otherwise, you may encounter compiler warnings or ignored attributes.
86-98: Consider Additional Error Documentation for Function Prototypes
While the function prototypes are clearly declared, consider adding short in-header documentation (e.g., with Doxygen-style comments) to clarify expected parameter ranges (e.g., valid year range) and error behavior. This will help maintain clarity for external integrators.Modelica/Utilities/Internal.mo (1)
214-221: Check for Missing Error Handling ingetNumberOfFiles
The external C call might fail if the directory does not exist or is inaccessible, but the function signature does not indicate how errors are handled or reported in Modelica. Consider providing at least a tested fallback or error message for invalid directories.Modelica/Resources/C-Sources/ModelicaTime.c (2)
49-71:epochFunction Handling Before 1970
The logic callsmktime(&tres)and raisesModelicaFormatErroron failure. As noted in prior discussions,mktimemay fail or behave unexpectedly for years before 1970 on certain OS/toolchains. If strictly needed, consider documenting or gracefully handling years < 1970.
105-155:ModelicaTime_localtimeThread Safety
Under some build conditions, the code can fall back tolocaltimerather thanlocaltime_rorlocaltime_s, which is not fully thread-safe. If you plan to call this function concurrently from multiple threads, consider reinforcing thread-safety at the platform-specific fallback level.Modelica/Utilities/Time.mo (4)
213-228: Lack of input validation in 'fromReadable'.
Similar to the previous comment, this constructor simply sets the fields without verification. If invalid day-month-year combinations are passed in, the record becomes inconsistent. Consider adding at least a documented disclaimer or minimal checks to gracefully handle erroneous inputs.
500-500: Grammar fix in documentation.
The doc string says "Check if DateTime dt1 is later as dt2," but standard phrasing is "later than dt2." Consider updating for clarity and consistency.- "Check if DateTime dt1 is later as dt2" + "Check if DateTime dt1 is later than dt2"
509-539: Consider simplifying the multi-level comparison logic.
These nested if-statements correctly compare date-time fields but are verbose. An alternative approach is to convert bothDateTimeobjects to a numeric epoch value (e.g., usingDateTime.epoch) and compare them directly. This may improve readability and consistency with other date/time operations.
637-644: Clarify doc for subtract_DateTime.
The documentation line says "Return time delta between dt2 and dt1 as Duration," whereas the function effectively calculates(dt1 - dt2). The call tofromDateTimes(dt2, dt1)is reversed to achieve that result, which can be confusing. Consider rewording the doc comment to explicitly mention that the function returnsdt1 - dt2or swapping the parameters for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.CI/Test/ModelicaTime.c(1 hunks).CI/Test/test.sh(2 hunks)Complex.mo(1 hunks)Modelica/Blocks/Math.mo(2 hunks)Modelica/Icons.mo(1 hunks)Modelica/Math/nearestInteger.mo(1 hunks)Modelica/Math/package.order(1 hunks)Modelica/Resources/BuildProjects/CMake/src.cmake(1 hunks)Modelica/Resources/BuildProjects/CMake/test.cmake(1 hunks)Modelica/Resources/BuildProjects/autotools/Makefile.am(1 hunks)Modelica/Resources/BuildProjects/gcc/Makefile(2 hunks)Modelica/Resources/C-Sources/ModelicaTime.c(1 hunks)Modelica/Resources/C-Sources/ModelicaTime.h(1 hunks)Modelica/Resources/C-Sources/readme.txt(2 hunks)Modelica/Resources/C-Sources/repl_str.h(1 hunks)Modelica/Resources/C-Sources/strptime.h(1 hunks)Modelica/Resources/Licenses/LICENSE_ModelicaTime.txt(1 hunks)Modelica/Resources/Licenses/Third-party/LICENSE_repl_str.txt(1 hunks)Modelica/Resources/Licenses/Third-party/LICENSE_strptime.txt(1 hunks)Modelica/Resources/Scripts/Conversion/ConvertModelica_from_4.0.0_to_5.0.0.mos(1 hunks)Modelica/Utilities/Internal.mo(3 hunks)Modelica/Utilities/System.mo(0 hunks)Modelica/Utilities/Time.mo(1 hunks)Modelica/Utilities/package.mo(2 hunks)Modelica/Utilities/package.order(1 hunks)ModelicaTest/Math.mo(1 hunks)ModelicaTest/Utilities.mo(2 hunks)ModelicaTestConversion5.mo(1 hunks)
💤 Files with no reviewable changes (1)
- Modelica/Utilities/System.mo
🧰 Additional context used
🧬 Code Graph Analysis (4)
.CI/Test/ModelicaTime.c (1)
Modelica/Resources/C-Sources/ModelicaTime.h (1)
ModelicaTime_strftime(91-93)
Modelica/Resources/C-Sources/repl_str.h (1)
Modelica/Resources/C-Sources/zlib/gzguts.h (2)
malloc(124-124)free(125-125)
Modelica/Resources/C-Sources/ModelicaTime.c (3)
Modelica/Resources/C-Sources/ModelicaTime.h (4)
ModelicaTime_difftime(86-87)ModelicaTime_localtime(88-90)ModelicaTime_strftime(91-93)ModelicaTime_strptime(94-97)Modelica/Resources/C-Sources/repl_str.h (2)
repl_str(18-109)repl_str(18-18)Modelica/Resources/C-Sources/strptime.h (2)
strptime_ms(98-520)strptime_ms(99-99)
Modelica/Resources/C-Sources/ModelicaTime.h (1)
Modelica/Resources/C-Sources/ModelicaTime.c (2)
ModelicaTime_difftime(73-103)char(157-219)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: external_c_checks_cmake_windows-msvc
🔇 Additional comments (39)
Modelica/Resources/Licenses/LICENSE_ModelicaTime.txt (1)
1-27: License file looks correct and appropriate.The license file uses a standard 3-clause BSD license, which is appropriate for the new ModelicaTime component. The copyright year range (2020-2024) is current and matches the project's conventions.
Modelica/Resources/C-Sources/readme.txt (2)
9-9: Addition of ModelicaTime.c is appropriate.The addition of ModelicaTime.c to the ModelicaExternalC library list is consistent with the PR's objective of adding date and time utilities.
57-57: Date updated appropriately.The date has been updated to reflect when these changes were made.
Modelica/Math/nearestInteger.mo (1)
1-44: Implementation of nearestInteger function looks correct.The function correctly implements the "round half away from zero" rule using the appropriate mathematical operations for both positive and negative numbers. The documentation is thorough with clear examples and proper references to related components.
Modelica/Icons.mo (2)
530-546: Formatting changes in Record class coordinates look good.The changes to remove decimal points from integer coordinates (e.g., changing from
origin={0.0,-25.0}toorigin={0,-25}) improve consistency and readability without affecting functionality.
553-582: Addition of OperatorRecord icon class is appropriate.The new OperatorRecord class provides a consistent visual representation for operator records, matching the existing design patterns in the library. This addition supports the PR objective of enhancing date and time utilities, which likely use operator records.
Modelica/Utilities/package.order (1)
7-7: LGTM! New Time package added to the package order.This addition aligns with the PR objectives to introduce new date and time utilities to the Modelica Standard Library. The Time package is correctly positioned in the package order file.
Modelica/Math/package.order (1)
13-13: LGTM! New nearestInteger function added to Math package.Adding the nearestInteger function to the Math package makes sense for time-related calculations. Based on the summary, this function will replace conditional logic using floor and ceil in the RealToInteger block, which is a good simplification.
Modelica/Resources/Licenses/Third-party/LICENSE_repl_str.txt (1)
1-5: LGTM! Proper license documentation for third-party code.The license file properly documents the usage of the repl_str function with appropriate attribution and licensing information. This is important for maintaining proper open-source compliance.
Modelica/Resources/Licenses/Third-party/LICENSE_strptime.txt (1)
1-26: License inclusion looks good.The BSD license for the strptime implementation from NetBSD Foundation is properly included. This satisfies legal requirements for incorporating third-party code.
Modelica/Resources/BuildProjects/CMake/test.cmake (1)
9-9: Appropriate test addition.Adding ModelicaTime to the MODELICA_TESTS list properly integrates the new time functionality into the testing framework.
Modelica/Resources/BuildProjects/gcc/Makefile (2)
14-15: ModelicaTime.o added correctly to object list.The ModelicaTime module has been properly added to the EXTC_OBJS list.
97-98: Compilation rule for ModelicaTime looks good.The compilation rule for ModelicaTime.o follows the same pattern as other modules.
Modelica/Resources/BuildProjects/CMake/src.cmake (1)
73-74: New time-related source files correctly added.The addition of ModelicaTime.c, ModelicaTime.h, repl_str.h, and strptime.h to the EXTC_SOURCES list properly integrates the new time functionality into the CMake build system.
The additions align with the PR objectives to enhance date and time functionalities in the Modelica Standard Library.
Also applies to: 76-76, 79-79
Complex.mo (1)
21-36: Improved icon graphics formatting for better readability.The restructuring of the Icon graphics annotation with proper indentation and line breaks makes the code more maintainable while maintaining the same functionality.
ModelicaTest/Math.mo (1)
36-41: Test cases added for the new nearestInteger function.These assertions properly verify the rounding behavior of the Math.nearestInteger function for different inputs, including positive and negative values, which ensures the function works as expected.
Note: There's a commented-out test for a border case (1.49999999999999999999) that would fail with the current implementation.
.CI/Test/ModelicaTime.c (1)
1-6: LGTM: Appropriate header files included.The includes are appropriate for a test file that will be checking the time-related functions in Modelica.
.CI/Test/test.sh (2)
13-14: Added static linking test for ModelicaTime.The ModelicaTime test is appropriately integrated into the static linking test section.
36-37: Added dynamic linking test for ModelicaTime.The ModelicaTime test is appropriately integrated into the dynamic linking test section.
Modelica/Resources/BuildProjects/autotools/Makefile.am (1)
2-2: Added ModelicaTime.c to the build system.The file
ModelicaTime.chas been added to the list of source files for thelibModelicaExternalClibrary. This addition is consistent with the PR objectives of enhancing date and time functionalities in the Modelica Standard Library.Modelica/Resources/C-Sources/repl_str.h (3)
1-6: Added string replacement utility with proper attribution.A string replacement utility function has been introduced with appropriate attribution to its original source and release information. This is good practice for incorporating third-party code.
7-17: Proper header guards and standard library inclusions.The header file includes proper header guards and includes the necessary standard libraries. The conditional inclusion of
stdint.hfor C99 or later is a good compatibility practice.
18-108: Well-implemented string replacement function.The
repl_strfunction is well-implemented with:
- Clear parameter documentation
- Configurable cache sizing parameters for performance tuning
- Proper memory management
- Handling of both C++ and C compilation
- Error handling for memory allocation failures
- Special case handling for when no replacements are needed
This implementation balances performance and memory usage effectively and will be useful for string manipulation in time formatting operations.
Modelica/Blocks/Math.mo (2)
1942-1942: Improved RealToInteger implementation using nearestInteger.The implementation of the RealToInteger block has been simplified by using the
Modelica.Math.nearestIntegerfunction instead of the previous conditional expression withfloorandceil. This change improves code maintainability and standardizes the rounding behavior.
1962-1966: Updated documentation to reflect implementation change.The documentation has been properly updated to reflect the implementation change, now referencing the
nearestIntegerfunction that's being used. This ensures consistency between implementation and documentation.ModelicaTestConversion5.mo (1)
23-36: Added proper package documentation and licensing information.The package includes appropriate annotations for version dependencies, copyright information, and licensing under the 3-Clause BSD license. The documentation clearly explains the purpose of this test package.
Modelica/Utilities/package.mo (2)
128-145: Clarity of acknowledgments looks good.The revised text properly credits additional contributors (Marco Kessler and Thomas Beutlich). This ensures transparency regarding project authorship.
214-215:✅ Verification successful
New
Timesublibrary reference.Thank you for explicitly documenting the new
Timesublibrary. This helps users discover the added date/time functionalities. Please verify that this link (modelica://Modelica.Utilities.Time) is fully functional across all supported tools.
🏁 Script executed:
#!/bin/bash # Checking for references to "Modelica.Utilities.Time" in the codebase: rg --context 3 "Modelica.Utilities.Time"Length of output: 21826
Link Verification Complete: New "Time" Sublibrary Reference is Consistent
Thank you for explicitly documenting the new
Timesublibrary. Our verification confirms that the link (modelica://Modelica.Utilities.Time) is referenced consistently across the codebase (e.g., inModelica/Utilities/package.mo,Modelica/Utilities/Time.mo, and related test files). There are no discrepancies observed in its usage that could impact functionality across supported tools.Modelica/Resources/C-Sources/strptime.h (1)
1-574: Overall implementation appears correct and complete.This file provides extensive parsing routines for date and time, including milliseconds support. A few considerations:
- Ensure that any null inputs (e.g., buf, fmt, or tm) are guarded or documented.
- Confirm thread-safety if these routines are used in concurrent contexts (static variables, global data, etc. are not modified).
- Validate that the license headers and disclaimers comply with your repository’s guidelines.
Otherwise, this looks solid and aligns with typical BSD-derived
strptimeimplementations.ModelicaTest/Utilities.mo (4)
393-437: FunctionTimetests look comprehensive.Good job covering dayOfWeek, leap year logic, and negative year range. These tests help validate the date/time utility thoroughly.
714-880:Durationconstructor and operator tests.All main duration operations are validated: addition, subtraction, comparisons, and formatting. Negative durations and boundary cases are handled. This coverage is solid.
387-387:❓ Verification inconclusive
Check cross-platform availability of
getPid.Using
Modelica.Utilities.System.getPid()is straightforward. Just ensure it behaves consistently on all supported platforms (Windows, Linux, macOS, etc.).
🏁 Script executed:
#!/bin/bash # Searching references and documentation for getPid usage rg "getPid"Length of output: 894
Revisit cross-platform consistency for
Modelica.Utilities.System.getPidusageOur grep results confirm that
getPidis defined and widely used throughout the Modelica library (see references inModelica/Utilities/System.mo, among others). However, note the comment in Modelica/Math/Random.mo mentioning thatgetPid(andgetTime) might not be available on some targets. Please double-check that the implementation ofModelica.Utilities.System.getPid()behaves consistently across all supported platforms (Windows, Linux, macOS, etc.). If there are any platform-specific limitations, consider adding conditional handling or updating the documentation accordingly.
439-713:✅ Verification successful
DateTimetests are well-structured.The coverage includes leap years, century logic, large date shifts, and negative epoch scenarios. One subtle point: confirm that the epoch offsets for certain far-future years (e.g., 2100) match actual calculations. Discrepancies could arise from manual or approximate references.
🏁 Script executed:
#!/bin/bash # Cross-check the stated epoch references for the 2100 date # Search for the lines referencing `2100, 2, 28` rg -A 5 "2100,\s*2,\s*28"Length of output: 362
Verification: DateTime Test Cases Confirmed
The DateTime tests are comprehensive, covering leap years, century rules, large date shifts, and negative epoch scenarios. The far-future case—with a DateTime of 2100-02-28 23:59:59 and an expected epoch offset of 951782399—is implemented using a custom reference epoch (epoch_year=2070) and the verification confirms that the code reflects this intended behavior.
Modelica/Resources/C-Sources/ModelicaTime.h (1)
1-30: File License Header Looks Good.
All standard clauses appear correctly outlined. No immediate changes needed.Modelica/Utilities/Internal.mo (3)
244-260: Validate Unit Test Coverage fordiffTime
The new functiondiffTimeis central to time offset computations. Consider verifying its behavior for edge cases like month = 2, day = 29, or years before 1970. This will help ensure reliability.Do you have test cases covering leap year transitions and earlier than 1970 date usage?
265-343: Impure FunctiongetTime
Calling an external function in an impure context is correct for retrieving system time. However, ensure that repeated calls in a tight loop are safe. Some embedded or specialized systems might have constraints on system time retrieval.
399-418:dayOfWeekAlgorithm Implementation
Tomohiko Sakamoto’s algorithm is a compact and efficient approach. Ensure you have tested at boundary years (e.g., 1900 or 2100) to confirm correct weekday calculations around century transitions, especially sincetm_yearoffsets might differ in some locales.Modelica/Resources/C-Sources/ModelicaTime.c (1)
73-103:ModelicaTime_difftimeSpecial Cases
The conditional avoidance ofmktimefor New Year 1970 is a clever workaround. However, be sure that drastically different time zones or daylight-saving boundaries near Jan 1, 1970 do not produce inconsistencies. You might consider a per-platform test harness around this logic.Modelica/Utilities/Time.mo (1)
200-207: Consider validating combined date fields.
The current approach neither validates that a provided day value is valid for its corresponding month nor checks year-month-day consistency (e.g., February 30). While this might be acceptable in Modelica or handled by external functions, adding an explicit validation (or documenting that invalid dates are unsupported) can reduce user errors.Would you like a verification script that searches the codebase for any usage of
DateTimethat might implicitly assume valid inputs?
| package ModelicaTestConversion5 | ||
| extends Modelica.Icons.ExamplesPackage; | ||
| package Utilities | ||
| extends Modelica.Icons.ExamplesPackage; | ||
| model Issue3247 "Conversion test for #3247" | ||
| extends Modelica.Icons.Example; | ||
| Integer ms; | ||
| Integer sec; | ||
| Integer min; | ||
| Integer hour; | ||
| Integer day; | ||
| Integer mon; | ||
| Integer year; | ||
| equation | ||
| (ms, sec, min, hour, day, mon, year) = Modelica.Utilities.System.getTime(); | ||
| annotation(experiment(StopTime=1), Documentation(info="<html> | ||
| <p> | ||
| Conversion test for <a href=\"https://github.com/modelica/ModelicaStandardLibrary/issues/3247\">#3247</a>. | ||
| </p> | ||
| </html>")); | ||
| end Issue3247; | ||
| end Utilities; |
There was a problem hiding this comment.
Created conversion test model for Issue #3247.
A test model has been created to verify the conversion script's handling of the changes to the time utilities. The model calls Modelica.Utilities.System.getTime() and assigns the returned values to time component variables.
However, this model may need to be updated since the PR indicates that getTime has been moved from the System package to a new Time package, and it now returns a record instead of individual values.
The conversion script should handle updating this call to use the new Time package and handle the returned record properly. Please verify that the conversion script correctly transforms:
(ms, sec, min, hour, day, mon, year) = Modelica.Utilities.System.getTime();to the new API format.
| void ModelicaTime_strptime(_Out_ int* ms, _Out_ int* sec, _Out_ int* min, | ||
| _Out_ int* hour, _Out_ int* mday, _Out_ int* mon, | ||
| _Out_ int* year, _In_z_ const char* buf, | ||
| _In_z_ const char* format) { | ||
| /* Formatted string to time conversion */ | ||
| #if defined(NO_TIME) | ||
| *ms = 0; | ||
| *sec = 0; | ||
| *min = 0; | ||
| *hour = 0; | ||
| *mday = 0; | ||
| *mon = 0; | ||
| *year = 0; | ||
| #else | ||
| struct tm tres; | ||
| int tmp = 0; | ||
|
|
||
| memset(&tres, 0, sizeof(struct tm)); | ||
| if (NULL != strptime_ms(buf, format, &tres, &tmp)) { | ||
| /* Do not memcpy as you do not know which sizes are in the struct */ | ||
| *ms = tmp; | ||
| *sec = tres.tm_sec; | ||
| *min = tres.tm_min; | ||
| *hour = tres.tm_hour; | ||
| *mday = tres.tm_mday; | ||
| *mon = 1 + tres.tm_mon; /* Correct for month starting at 1 */ | ||
| *year = 1900 + tres.tm_year; /* Correct for 4-digit year */ | ||
| } | ||
| else { | ||
| *ms = 0; | ||
| *sec = 0; | ||
| *min = 0; | ||
| *hour = 0; | ||
| *mday = 0; | ||
| *mon = 0; | ||
| *year = 0; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graceful Failure Handling in ModelicaTime_strptime
Upon parsing failure, the function sets all outputs to 0. Consider logging an error or providing a conditional return code for better traceability, especially if invalid user input is expected or more advanced error handling is desired.
| _Ret_z_ const char* ModelicaTime_strftime(int ms, int sec, int min, int hour, | ||
| int mday, int mon, int year, | ||
| _In_z_ const char* format, int _maxSize) { | ||
| /* Formatted time to string conversion */ | ||
| #if defined(NO_TIME) | ||
| return ""; | ||
| #else | ||
| struct tm tres; | ||
| const size_t maxSize = (size_t)_maxSize; | ||
| char* timePtr = ModelicaAllocateString(maxSize); | ||
| time_t calendarTime = epoch(sec, min, hour, mday, mon, year); | ||
| #if !defined(_POSIX_) && !(defined(_MSC_VER) && _MSC_VER >= 1400) | ||
| struct tm* tlocal; | ||
| #endif | ||
|
|
||
| #if defined(_POSIX_) | ||
| localtime_r(&calendarTime, &tres); /* Time fields in local time zone */ | ||
| #elif defined(_MSC_VER) && _MSC_VER >= 1400 | ||
| localtime_s(&tres, &calendarTime); /* Time fields in local time zone */ | ||
| #else | ||
| tlocal = localtime(&calendarTime); /* Time fields in local time zone */ | ||
| tres = *tlocal; | ||
| #endif | ||
|
|
||
| { | ||
| char* format2; | ||
| char* format3; | ||
| char ms_str[32]; | ||
| size_t retLen; | ||
|
|
||
| /* Special handling of milliseconds by non-standard %L format specifier */ | ||
| format2 = repl_str(format, "%%", "%|"); | ||
| sprintf(ms_str, "%03d", ms); | ||
| format3 = repl_str(format2, "%L", ms_str); | ||
| free(format2); | ||
| format2 = repl_str(format3, "%|", "%%"); | ||
| free(format3); | ||
|
|
||
| #if !defined(NO_LOCALE) && (defined(_MSC_VER) && _MSC_VER >= 1400) | ||
| { | ||
| _locale_t loc = _create_locale(LC_TIME, "C"); | ||
| retLen = _strftime_l(timePtr, maxSize, format2, &tres, loc); | ||
| _free_locale(loc); | ||
| } | ||
| #elif !defined(NO_LOCALE) && (defined(__GLIBC__) && defined(__GLIBC_MINOR__) && ((__GLIBC__ << 16) + __GLIBC_MINOR__ >= (2 << 16) + 3)) | ||
| { | ||
| locale_t loc = newlocale(LC_TIME, "C", NULL); | ||
| retLen = strftime_l(timePtr, maxSize, format2, &tres, loc); | ||
| freelocale(loc); | ||
| } | ||
| #else | ||
| retLen = strftime(timePtr, maxSize, format2, &tres); | ||
| #endif | ||
| free(format2); | ||
| if (retLen > 0 && retLen <= maxSize) { | ||
| return (const char*)timePtr; | ||
| } | ||
| else { | ||
| return ""; | ||
| } | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Memory Leak Risk on Formatting Failure
When retLen is zero or exceeds maxSize, the function returns "" (line 215) without freeing timePtr. This leads to a memory leak. Consider freeing timePtr before returning, or returning a dynamically allocated empty string that is freed by the caller.
- return "";
+ free(timePtr);
+ return "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _Ret_z_ const char* ModelicaTime_strftime(int ms, int sec, int min, int hour, | |
| int mday, int mon, int year, | |
| _In_z_ const char* format, int _maxSize) { | |
| /* Formatted time to string conversion */ | |
| #if defined(NO_TIME) | |
| return ""; | |
| #else | |
| struct tm tres; | |
| const size_t maxSize = (size_t)_maxSize; | |
| char* timePtr = ModelicaAllocateString(maxSize); | |
| time_t calendarTime = epoch(sec, min, hour, mday, mon, year); | |
| #if !defined(_POSIX_) && !(defined(_MSC_VER) && _MSC_VER >= 1400) | |
| struct tm* tlocal; | |
| #endif | |
| #if defined(_POSIX_) | |
| localtime_r(&calendarTime, &tres); /* Time fields in local time zone */ | |
| #elif defined(_MSC_VER) && _MSC_VER >= 1400 | |
| localtime_s(&tres, &calendarTime); /* Time fields in local time zone */ | |
| #else | |
| tlocal = localtime(&calendarTime); /* Time fields in local time zone */ | |
| tres = *tlocal; | |
| #endif | |
| { | |
| char* format2; | |
| char* format3; | |
| char ms_str[32]; | |
| size_t retLen; | |
| /* Special handling of milliseconds by non-standard %L format specifier */ | |
| format2 = repl_str(format, "%%", "%|"); | |
| sprintf(ms_str, "%03d", ms); | |
| format3 = repl_str(format2, "%L", ms_str); | |
| free(format2); | |
| format2 = repl_str(format3, "%|", "%%"); | |
| free(format3); | |
| #if !defined(NO_LOCALE) && (defined(_MSC_VER) && _MSC_VER >= 1400) | |
| { | |
| _locale_t loc = _create_locale(LC_TIME, "C"); | |
| retLen = _strftime_l(timePtr, maxSize, format2, &tres, loc); | |
| _free_locale(loc); | |
| } | |
| #elif !defined(NO_LOCALE) && (defined(__GLIBC__) && defined(__GLIBC_MINOR__) && ((__GLIBC__ << 16) + __GLIBC_MINOR__ >= (2 << 16) + 3)) | |
| { | |
| locale_t loc = newlocale(LC_TIME, "C", NULL); | |
| retLen = strftime_l(timePtr, maxSize, format2, &tres, loc); | |
| freelocale(loc); | |
| } | |
| #else | |
| retLen = strftime(timePtr, maxSize, format2, &tres); | |
| #endif | |
| free(format2); | |
| if (retLen > 0 && retLen <= maxSize) { | |
| return (const char*)timePtr; | |
| } | |
| else { | |
| return ""; | |
| } | |
| } | |
| #endif | |
| } | |
| _Ret_z_ const char* ModelicaTime_strftime(int ms, int sec, int min, int hour, | |
| int mday, int mon, int year, | |
| _In_z_ const char* format, int _maxSize) { | |
| /* Formatted time to string conversion */ | |
| #if defined(NO_TIME) | |
| return ""; | |
| #else | |
| struct tm tres; | |
| const size_t maxSize = (size_t)_maxSize; | |
| char* timePtr = ModelicaAllocateString(maxSize); | |
| time_t calendarTime = epoch(sec, min, hour, mday, mon, year); | |
| #if !defined(_POSIX_) && !(defined(_MSC_VER) && _MSC_VER >= 1400) | |
| struct tm* tlocal; | |
| #endif | |
| #if defined(_POSIX_) | |
| localtime_r(&calendarTime, &tres); /* Time fields in local time zone */ | |
| #elif defined(_MSC_VER) && _MSC_VER >= 1400 | |
| localtime_s(&tres, &calendarTime); /* Time fields in local time zone */ | |
| #else | |
| tlocal = localtime(&calendarTime); /* Time fields in local time zone */ | |
| tres = *tlocal; | |
| #endif | |
| { | |
| char* format2; | |
| char* format3; | |
| char ms_str[32]; | |
| size_t retLen; | |
| /* Special handling of milliseconds by non-standard %L format specifier */ | |
| format2 = repl_str(format, "%%", "%|"); | |
| sprintf(ms_str, "%03d", ms); | |
| format3 = repl_str(format2, "%L", ms_str); | |
| free(format2); | |
| format2 = repl_str(format3, "%|", "%%"); | |
| free(format3); | |
| #if !defined(NO_LOCALE) && (defined(_MSC_VER) && _MSC_VER >= 1400) | |
| { | |
| _locale_t loc = _create_locale(LC_TIME, "C"); | |
| retLen = _strftime_l(timePtr, maxSize, format2, &tres, loc); | |
| _free_locale(loc); | |
| } | |
| #elif !defined(NO_LOCALE) && (defined(__GLIBC__) && defined(__GLIBC_MINOR__) && ((__GLIBC__ << 16) + __GLIBC_MINOR__ >= (2 << 16) + 3)) | |
| { | |
| locale_t loc = newlocale(LC_TIME, "C", NULL); | |
| retLen = strftime_l(timePtr, maxSize, format2, &tres, loc); | |
| freelocale(loc); | |
| } | |
| #else | |
| retLen = strftime(timePtr, maxSize, format2, &tres); | |
| #endif | |
| free(format2); | |
| if (retLen > 0 && retLen <= maxSize) { | |
| return (const char*)timePtr; | |
| } | |
| else { | |
| free(timePtr); | |
| return ""; | |
| } | |
| } | |
| #endif | |
| } |
There was a problem hiding this comment.
timePtr was allocated by ModelicaAllocateString and thus its lifetime is managed by the runtime. It must not be freed using free.
There was a problem hiding this comment.
@beutlich Thank you for the correction. You're absolutely right - memory allocated with ModelicaAllocateString is managed by the runtime and should not be manually freed with free(). My suggestion was incorrect and could have introduced memory corruption issues. I appreciate you clarifying this important distinction about the memory management approach in this codebase.
✏️ Learnings added
Learnt from: beutlich
PR: modelica/ModelicaStandardLibrary#3247
File: Modelica/Resources/C-Sources/ModelicaTime.c:157-219
Timestamp: 2025-04-11T16:43:22.169Z
Learning: In Modelica, memory allocated with `ModelicaAllocateString` is managed by the runtime system and must not be freed manually using `free()`. Attempting to free this memory manually would lead to memory corruption or double-free errors.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
New Features
Improvements
Tests