- 
                Notifications
    
You must be signed in to change notification settings  - Fork 175
 
Add ModelicaDuplicateString and ModelicaDuplicateStringWithErrorReturn (introduced in Modelica 3.5) #3686
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
Conversation
78607f6    to
    b4772b7      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, assuming you looked at all occurrences of ModelicaAllocateString and ModelicaAllocateStringWithErrorReturn to find all that should be replaced by the new functions.
By the way, I noticed that the test implementations of the old functions still don't add a null terminator. Is there a plan to also update these (and remove any redundant null termination on the results of these calls)?
          
 You mean ModelicaAllocateString beahves like calloc? I missed that change.  | 
    
          
 Not exactly. It allows for detection of uninitialized reads by only zeroing the last byte.  | 
    
4896289    to
    95a12fb      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking accidental merge. Need to wait until master is based on MLS 3.56.
95a12fb    to
    c4381bc      
    Compare
  
    
          
 This has now been changed (to 3.6 to be specific).  | 
    
          
 So, others features than 3.6 annotations are desired, right?  | 
    
          
 Yes. Tool vendors will need to react and bring up a discussion if they think the adoption of a particular new feature (such as these string allocation functions) is too fast.  | 
    
| 
           It would help if there was a formal decision what MSL version would now allow MLS 3.6 features so the milestone of this PR can be set accordingly.  | 
    
          
 There was (at the monthly MAP-Lib meeting on the 14th of November, if I remember the date right). 4.1.0 will allow 3.6 features. As Henrik says, if any tool vendor thinks a specific feature from 3.6 is not ready to be used, they can speak up in the corresponding pull request.  | 
    
The review from 2021 predates the decision to base the next release of MSL on Modelica 3.6; thus it is no longer relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
c4381bc    to
    adc1f12      
    Compare
  
    
          
 I detected somemore occurrences after rebasing, see adc1f12.  | 
    
Co-authored-by: Henrik Tidefelt <[email protected]>
Co-authored-by: Henrik Tidefelt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that it matters a whole lot, but I think it would be possible to call ModelicaError (which ends with assert(0) anyways) as promised in case of error.
          
 Thanks. All done. Please cross-check again!  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Can only be merged once MSL is based on MLS 3.5.