SystemRDL / PeakRDL-ipxact

Import and export IP-XACT XML register models
http://peakrdl-ipxact.readthedocs.io
GNU General Public License v3.0
31 stars 10 forks source link

Handle cases where legal IP-XACT names are not legal SystemRDL names #8

Closed jasonpjacobs closed 2 years ago

jasonpjacobs commented 2 years ago

The IP-XACT spec (1685-2014, Section D.7) has the following description of allowed characters in names:

The Name type defines a series of any characters, excluding embedded whitespace. It needs to begin with a letter, colon (:), or underscore (). A Name shall contain only letters, numbers, and the colon (:), underscore (), dash (-), and dot (.) characters. Any leading or trailing spaces are removed. This seems like a straightforward fix that I may take on if it isn't already planned.

However, SystemRDL's rules about identifiers are more strict:

An identifier assigns a name to a user-defined data type or its instance. There are two types of identifiers: simple and escaped. Identifiers are case-sensitive. Simple identifiers have a first character that is a letter or underscore (_) followed by zero or more letters, digits, and underscores. Escaped identifiers begin with \ followed by a simple identifier.

Note, escaped identifiers allow one to use a SystemRDL keyword as an identifier, but don't allow the use of dots, dashes, colons, etc.

I'm happy to author a pull request to implement this. Writing a function/method to translate an IP-XACT name to a valid SystemRDL one is straightforward. Where might be the best place to introduce this feature? Users will likely want to control whether the transformation is performed as well as the approach taken. I think either a class method that can be overridden, or a stand alone function that is passed into importer's initializer. Thoughts?

amykyta3 commented 2 years ago

Fixed and published v2.1.2

jasonpjacobs commented 2 years ago

Wow.. thanks for the quick response and commit! Can we expose an initializer argument that controls this feature? There are cases where I'll want to perform the name sanitization, and other cases were importing incompatible names should raise an exception.

amykyta3 commented 2 years ago

Totally agree on that point. I was just thinking about that today too - there ought to be a way to let the user implement their own sanitization function. I've often encountered files from 3rd party vendors that like to do all sorts of shenanigans like prefixing every register/field with some junk. Would love to delete that.

I'll think of a good way to do this. I'll probably break out the sanitize operation into a separate function so that it can be overridden by extending the class. Would be nice to know the situational context. Some vendors like to prefix the register name to all the fields which is pretty grating.

On Fri, Jan 14, 2022 at 3:46 PM Jason Jacobs @.***> wrote:

Wow.. thanks for the quick response and commit! Can we expose an initializer argument that controls this feature? There are cases where I'll want to perform the name sanitization, and other cases were importing incompatible names should raise an exception.

— Reply to this email directly, view it on GitHub https://github.com/SystemRDL/PeakRDL-ipxact/issues/8#issuecomment-1013546178, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3W6I37KEWL6ES7DGSF5BTUWCYV5ANCNFSM5L2DLN4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.***>

jasonpjacobs commented 2 years ago

How about giving users the ability to provide a callable to use? The library itself can provide a function similar to what you've done already. But if for example I wanted to keep track of the names changes that were made I could create a class with a call method that returns the sanitized name, and in the implementation store a mapping of the original name to the mapped name.

amykyta3 commented 2 years ago

Recommend simply extending the exporter class and enhancing the function itself