-
Notifications
You must be signed in to change notification settings - Fork 15
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
Allow char literal convertion into unsigned #82
Allow char literal convertion into unsigned #82
Conversation
required to compile stm32mp1xxx dtb
It looks fine, but let's wait for a review. |
Thanks. I am happy to merge this, but I'd prefer if we handled common escape characters in character literals first. |
input_buffer.cc
Outdated
outInt = (unsigned char)'\r'; | ||
break; | ||
default: | ||
// Let any other symbol to resolves to itself. |
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 sure about this. Just think it is better to allow.
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.
It would be good to handle \, ' and \t here as well. Anything else should probably be an error. In particular, at some point someone might decide that they want to write '\x1234' and we should report that this is not something that we're handling.
This also assumes single-byte characters, but I'm happy with that for now because otherwise we may have to think about endian issues.
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.
ok.
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.
Can you also add a test? Tests for things like this are simple: just a .dts file using the feature and a .dts.expected file that contains the expected output (you can generate this with -I dts -O dts
and visually check the output).
input_buffer.cc
Outdated
outInt = (unsigned char)'\r'; | ||
break; | ||
default: | ||
// Let any other symbol to resolves to itself. |
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.
It would be good to handle \, ' and \t here as well. Anything else should probably be an error. In particular, at some point someone might decide that they want to write '\x1234' and we should report that this is not something that we're handling.
This also assumes single-byte characters, but I'm happy with that for now because otherwise we may have to think about endian issues.
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.
I think, '\0' may be supported right now, before octal or hex escapes implemented.
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.
I assume, all commits will be squashed when merging this PR.
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.
Thanks, looks good.
I can squash and merge, unless you want to tidy up the history first.
break; | ||
case '\'': | ||
case '\\': | ||
break; |
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.
Minor style nit, but it’s good to put default first so that nothing accidentally falls through into it.
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.
ok. minute
b722b45
to
db2bc48
Compare
please squash & merge. I prefer to keep history |
Thanks! |
Thank you! |
required to compile stm32mp1xxx dtb