Binary function names by zummenix · Pull Request #91 · bytecodealliance/cranelift

zummenix · 2017-06-04T08:37:46Z

Eventually will close #47

I hope I move in right direction.

stoklund

Thanks for working on this!

All your changes to the file tests and unit tests look good.

The FunctionName implementation should be able to print itself, using the %nnn syntax when possible, and falling back to the #xxxx syntax otherwise.

It would be good to add some test containing Unicode alphabetic characters, just to make sure they are not printed with %nnn.

stoklund · 2017-06-05T15:22:19Z

lib/cretonne/src/write.rs

    fn basic() {
        let mut f = Function::new();
-        assert_eq!(f.to_string(), "function \"\"() {\n}\n");
+        assert_eq!(f.to_string(), "function %() {\n}\n");


Yep. I think this is an OK syntax for the empty function name.

stoklund · 2017-06-05T15:30:32Z

lib/cretonne/src/ir/funcname.rs

+    ///
+    /// Caller should validate that the string contains only
+    /// ASCII alphanumerical characters and `_`.
+    pub fn from_string(s: &str) -> FunctionName {


I think with_str would be a better name for this constructor. Thefrom_string name implies a conversion constructor which would be expected to consume a String. See the Rust naming conventions.

I don't think the comment is correct. The caller should be allowed to pass any string, and it will be printed in #xxx form if it is not pure ASCII.

stoklund · 2017-06-05T15:34:25Z

lib/cretonne/src/ir/funcname.rs

-            f.write_str(&self.0)
-        }
+        f.write_char('%')?;
+        f.write_str(&self.0)


Perhaps easier: write!(f, "%{}", self.0).

I would like for the Display implementation to still detect if the function name is pure ASCII so it can be printed with the % syntax. If it can't, it should be printed in hexadecimal instead with #xxxx. The legal characters in the %xx notating is ASCII alphanumerical characters and _.

stoklund · 2017-06-05T15:37:23Z

lib/cretonne/src/ir/funcname.rs

+    fn formatting_string() {
+        assert_eq!(FunctionName::from_string("").to_string(), "%");
+        assert_eq!(FunctionName::from_string("x").to_string(), "%x");
+        assert_eq!(FunctionName::from_string(" ").to_string(), "% ");


The first two are correct, the second one should print as #20.

stoklund · 2017-06-05T15:45:00Z

BTW, some of this may be easier if you switch the internal representation from a String to a Vec<u8>. Then you won't have to worry about invalid UTF-8 etc.

It's still useful for the parser to have a with_str constructor that takes a UTF-8 &str like the one you added. See str::as_bytes().

stoklund · 2017-06-05T15:45:21Z

Sorry, didn't mean to close this!

zummenix · 2017-06-05T17:05:26Z

Thank you for the review!

Initially I thought that FunctionName will be an enum with cases like Name and Hex since lexer recognises %nnnn or #xxxx and these sequences are valid to create FunctionName.

So my next actions are:

FunctionName will accept any bytes (two constructors new and with_str).
for Token::Name we'll convert str::as_bytes()
for Token::HexSequence we'll convert from hex to bytes
If FunctionName can Display itself as %nnnn it will, if not it'll fallback to #xxxx

Is that correct?

stoklund · 2017-06-05T17:30:17Z

Yes, you got it right

zummenix · 2017-06-06T16:30:38Z

@stoklund, It would be helpful to have some dependency or module that can convert to/from hex. I'm hesitant to copy code from other crates.

stoklund · 2017-06-06T16:43:59Z

lib/cretonne/src/ir/funcname.rs

-fn is_id_continue(c: char) -> bool {
-    c.is_ascii() && (c == '_' || c.is_alphanumeric())
+    /// Creates a new function name from a sequence of bytes.
+    pub fn new(bytes: Vec<u8>) -> FunctionName {


I think you should just take a bytes: &[u8] slice here. We don't want to force the caller to allocate a Vec.

Let's leave one generic constructor:pub fn new<T>(v: T) -> FunctionName where T: Into<Vec<u8>> ?

Do you mean in addition to a &[u8] constructor? That would be OK.

No, I mean only one constructor. That way we can create FunctionName using &str and &[u8]

I see what you mean. As long as we can construct from both an &str and an &[u8], that is fine.

stoklund · 2017-06-06T16:45:12Z

lib/cretonne/src/ir/funcname.rs

+            f.write_char('#')?;
+            for i in self.0.iter().map(|&b| b as usize) {
+                f.write_char(HEX_CHARS[i >> 4] as char)?;
+                f.write_char(HEX_CHARS[i & 0xf] as char)?;


You can print a hexadecimal byte like this:

write!(f, "{:02x}", b)?

Thanks, didn't think of it.

stoklund · 2017-06-06T16:52:46Z

For parsing the hexadecimal string, you can use u8::from_str_radix() after you split the string into two-character parts.

stoklund

This looks great. Thanks!

stoklund suggested changes Jun 5, 2017

View reviewed changes

stoklund closed this Jun 5, 2017

stoklund reopened this Jun 5, 2017

stoklund reviewed Jun 6, 2017

View reviewed changes

zummenix changed the title ~~[WIP] Binary function names~~ Binary function names Jun 10, 2017

zummenix added 9 commits June 10, 2017 21:02

Function names should start with %

0aafcf3

Create FunctionName from string

c22b5e0

Implement displaying of FunctionName as %nnnn with fallback to #xxxx

58841de

Run rustfmt and fix FunctionName::with_string in parser

7e09575

Implement FunctionName::new as a generic function

4434dd9

Binary function names should start with #

5a3dcc5

Implement NameRepr for function name

85187c8

Fix examples in docs to reflect that function names start with %

15a39a0

Rebase and fix filecheck tests

2bec450

stoklund approved these changes Jun 10, 2017

View reviewed changes

stoklund merged commit 8b484b1 into bytecodealliance:master Jun 10, 2017

zummenix deleted the feature/#47-binary-function-names branch June 11, 2017 04:28

Conversation

zummenix commented Jun 4, 2017

Uh oh!

stoklund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoklund commented Jun 5, 2017

Uh oh!

stoklund commented Jun 5, 2017

Uh oh!

zummenix commented Jun 5, 2017

Uh oh!

stoklund commented Jun 5, 2017

Uh oh!

zummenix commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoklund commented Jun 6, 2017

Uh oh!

stoklund left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants