Skip to content

Name mangling is lossy around numbers #76

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

Closed
whitequark opened this issue Apr 29, 2017 · 5 comments
Closed

Name mangling is lossy around numbers #76

whitequark opened this issue Apr 29, 2017 · 5 comments
Milestone

Comments

@whitequark
Copy link
Contributor

E.g. both ADC_SPC_PHASE_22_5 and ADC_SPC_PHASE_225 converge to AdcSpcPhase225.

@whitequark
Copy link
Contributor Author

whitequark commented Apr 29, 2017

I use the following patch to the inflections crate as a workaround:

diff --git a/src/case.rs b/src/case.rs
index 0c6bea7..a22677f 100644
--- a/src/case.rs
+++ b/src/case.rs
@@ -443,6 +443,12 @@ pub fn is_constant_case(string: &str) -> bool {
   string == to_constant_case(string)
 }
 
+/// Checks if a character is a digit.
+#[inline]
+fn is_digit(c: char) -> bool {
+  c >= '0' && c <= '9'
+}
+
 /// Checks if a character is a separator.
 #[inline]
 fn is_separator(c: char) -> bool {
@@ -472,7 +478,15 @@ fn scan_to_camel(state: &mut (bool, Option<char>), curr: char) -> Option<String>
   let last = state.1;
   state.1 = Some(curr);
 
-  if state.0 {
+  if state.0 && is_digit(curr) {
+    // If the state has signaled the next character must be capitalized,
+    // but it is a digit, then prepend a separator to it.
+    state.0 = false;
+    let mut string = String::with_capacity(2);
+    string.push('_');
+    string.push(curr);
+    Some(string)
+  } else if state.0 {
     // If the state has signaled the next character must be capitalized,
     // capitalize it and mark the state as finished.
     state.0 = false;

It is not obvious how or where it should be integrated.

@whitequark
Copy link
Contributor Author

Note: this is not perfect and results in errors such as follows:

error: variant `CcmCrcctrlInit_0` should have a camel case name such as `Ccmcrcctrlinit0`

@whitequark
Copy link
Contributor Author

whitequark commented Apr 29, 2017

The more I think about it, the more I realize that name mangling of types (well, any name mangling that changes separators) is a bad idea.

  1. It creates complications like this issue.
  2. It is literally impossible to satisfy the non_camel_case_types lint without destroying semantics sometimes, because names like Foo4_20 are invalid:
    warning: type `Foo4_20` should have a camel case name such as `Foo420`
  3. The specific naming of the type matters rarely because it never appears in user's code.
  4. When the type name does matter is when it appears in the error messages, and I cannot in good faith describe names like SysctlSri2cR0 as in any way easy to read. It simply goes against any established practice in embedded dev.
  5. It is not (visually or otherwise) symmetric with other names. E.g. uart_ibrd does not stand out as the counterpart of UartIbrd. Typical Rust code rarely has a need for having this symmetry (and rarely uses as many acronyms), but this is not typical Rust code.

Therefore I propose to allow non-camel-case types and stop this mangling altogether.

@Emilgardis
Copy link
Member

Emilgardis commented Apr 30, 2017

All <name> tags are supposed to follow C ansi code standards. Code gen from SVD's usually use macro naming conventions, i.e exactly what <name> holds. This issue is a tricky one to fix.

Can one possibly make 22_5 into something that conveys the implicit .?

@whitequark
Copy link
Contributor Author

Can one possibly make 22_5 into something that conveys the implicit .?

One could of course mangle the SVD files in any applicable way to work around a deficiency of svd2rust but I don't think this should be necessary.

@japaric japaric added this to the cmsis-svd milestone May 25, 2017
japaric pushed a commit that referenced this issue Jun 5, 2017
Normalize SVD names to upper case where they were to pascal case

Fixes #76.

Depends on calebmer/inflections#2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants