scripts: dts: properly escape string properties #78433
Conversation
|
Hello @joelspadin, and thank you very much for your first pull request to the Zephyr project! |
|
Can you add testing for thist? |
|
Sure. I'm not familiar with the tests for this area of the code though. Can you point me to the right location for the tests? |
Take a look at |
|
I have added new test cases. I covered all the standard C escapes (except |
| zassert_str_equal(DT_PROP_BY_IDX(DT_NODELABEL(test_stra_escape), val, 2), "first\nsecond"); | ||
| zassert_str_equal(DT_PROP_BY_IDX(DT_NODELABEL(test_stra_escape), val, 3), "\x01\x7F"); | ||
| } | ||
|
|
There was a problem hiding this comment.
why no testing of DT_STRING_UNQUOTED?
There was a problem hiding this comment.
Simply because I couldn't think of any reason you would ever want to use DT_STRING_UNQUOTED with these characters, since from the documentation it seems to be primarily for turning strings into C code, which typically would not use these characters.
Something like
val = "\"first\" SECOND \"third\"";
#define SECOND " second "
zassert_str_equal(DT_STRING_UNQUOTED(node, val), "first second third")could be a possible use for this though, so I will add some tests for it.
There was a problem hiding this comment.
yes, I think since you're going through the effort to make sure the generation of this macro handles the escaped strings, might as well add a test so that we are at least clear on the behavior for the future so that we can have some definition of what "handling" means
scripts/dts/gen_defines.py
Outdated
| # Prepend a line continuation character to any new lines in 's' so all the | ||
| # lines are kept together in a #define value. | ||
|
|
||
| return s.replace("\r", "\\\r").replace("\n", "\\\n") |
There was a problem hiding this comment.
to be clear, if the property is "first\nsecond", then the macro for string_unquoted version will be firstsecond with this logic. is this intentional?
There was a problem hiding this comment.
Looking at this again, I realized that this doesn't actually insert a line break into the expanded macro, because that is not possible to begin with.
Would inserting a space be a reasonable alternative? If I understand this correctly, it is used to expand the string into something that can be evaluated as C code, in which case all that matters is that there is whitespace, not what kind.
There was a problem hiding this comment.
I'm not really sure what it should be, since I don't know a use case where you would or wouldn't want to know about a newline in the unquoted version of the string, I just wanted to be clear if you were aware that's what it was doing. I just don't think we should leave the behavior undefined
There was a problem hiding this comment.
Given the limitations of C macros, I don't believe it's possible to know about a new line in the unquoted string. I've changed it to be replaced by a space and added some tests for using \n and \" in unquoted strings.
I cannot think of any reason you would intentionally use \n with the unquoted string API, but it at the very least needs to generate code that compiles so it can be used in quoted strings, so I just duplicated one of the existing tests but with \n in place of spaces.
\" is plausible to intentionally use with the unquoted API, since it could be used for joining multiple string literals, so I've included a test for that as well.
joelspadin
force-pushed
the
string-property-escaping
branch
from
41742ce to
1b9e8fd
Compare
joelspadin
force-pushed
the
string-property-escaping
branch
from
1b9e8fd to
740b7f7
Compare
|
Rebased and resolved the conflict. Also added type annotations to new functions to match the changes I rebased onto. |
Fixed escaping of double quotes, backslashes, and new line characters so they can be used in string properties. Previously, double quotes and backslashes were escaped in gen_defines.py but not in gen_dts_cmake.py, and new lines were not escaped in either, so using any of these characters would break the build. Signed-off-by: Joel Spadin <[email protected]>
Added tests for escape sequences in string and string-array properties. Signed-off-by: Joel Spadin <[email protected]>
joelspadin
force-pushed
the
string-property-escaping
branch
from
740b7f7 to
e441be7
Compare
|
Hi @joelspadin! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
Fixed escaping of double quotes, backslashes, and new line characters so they can be used in string properties.
Previously, double quotes and backslashes were escaped in gen_defines.py but not in gen_dts_cmake.py, and new lines were not escaped in either, so using any of these characters would break the build. These characters aren't typically used in devicetree (I found this while working on a feature for ZMK, whose keymaps system is probably an atypical use of devicetree), but I don't see why they shouldn't be supported.
This may conflict with #78159, but the conflicts look minimal.