Skip to content

sys/string_utils: add string_writer helper#20843

Merged
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:string_writer
Dec 20, 2024
Merged

sys/string_utils: add string_writer helper#20843
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:string_writer

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Aug 29, 2024

Contribution description

Assembling a string through multiple writes if often open-coded like this

char buffer[64];
size_t len = 0;
for (unsigned i = 0; i < ARRAY_SIZE(names); ++i) {
    len += snprintf(&buffer[len], sizeof(buffer) - len, "%u: %s\n", i, names[i]);
}

This is error prone (the above example foregoes any error handling) and ugly.

This introduces a small helper function to achieve the same in a much more convenient fasion:

char buffer[64];
string_writer_t sw;
string_writer_init(&sw, buffer, sizeof(buffer));

for (unsigned i = 0; i < ARRAY_SIZE(names); ++i) {
    swprintf(&sw, "%u: %s\n", i, names[i]);
}

Testing procedure

A new unit test has been added.

Issues/PRs references

@benpicco benpicco requested a review from miri64 as a code owner August 29, 2024 12:04
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Aug 29, 2024
@benpicco benpicco requested review from fabian18 and maribu August 29, 2024 12:04
@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 29, 2024

That API indeed disarms the usual footguns :)

I wonder if there is some BSD / whatever API already that we could steal?

aabadie
aabadie reviewed Aug 29, 2024
View reviewed changes
Teufelchen1
Teufelchen1 requested changes Sep 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all looks good. Some nits, some ideas.

You mentioned that this is often done manually in RIOT, could you also replace such instances with this helper right away?

Teufelchen1
Teufelchen1 requested changes Sep 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements.

@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Sep 10, 2024

Squash & Ci?

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 10, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Sep 10, 2024 •
edited
Loading

Murdock results

✔️ PASSED

db6196e tests/pkg/relic: blacklist ATXmega boards

Success Failures Total Runtime
10242 0 10246 13m:37s

Artifacts

@benpicco benpicco force-pushed the string_writer branch 2 times, most recently from ce88749 to 0fe3ac4 Compare September 10, 2024 10:09
derMihai
derMihai reviewed Sep 16, 2024
View reviewed changes
sys/include/string_utils.h Outdated
* @return number of bytes written on success
* -E2BIG if the string was truncated
*/
int swprintf(string_writer_t *sw, const char *restrict format, ...);
Copy link
Copy Markdown
Contributor

@derMihai derMihai Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add

__attribute__ ((format (printf, 2, 3)))

to enable static printf-like compiler checks for the format string and arguments, see https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html.

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC doesn't like it if the variable args are empty

/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c: In function ‘test_libc_swprintf’:
/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c:42:5: error: format not a string literal and no format arguments [-Werror=format-security]
     res = swprintf(&sw, "Hello World!");
     ^~~

Copy link
Copy Markdown
Contributor

@derMihai derMihai Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd, I tried now and works for me:

__attribute__ ((format (printf, 2, 3)))
int __swprintf(string_writer_t *sw, FLASH_ATTR const char *restrict format, ...);

Then:

    res = swprintf(&sw, "Hello World!");

works just fine, while:

    res = swprintf(&sw, "Hello World%s!");

results in

/home/[email protected]/proj/RIOT/tests/unittests/tests-libc/tests-libc.c: In function ‘test_libc_s
wprintf’:
/home/[email protected]/proj/RIOT/tests/unittests/tests-libc/tests-libc.c:42:38: error: format ‘%s’
 expects a matching ‘char *’ argument [-Werror=format=]
   42 |     res = swprintf(&sw, "Hello World%s!");
      |                                     ~^
      |                                      |
      |                                      char *

Which compiler version are you using? Mine:

$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:10.3-2021.07-4) 10.3.1 20210621 (release)

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only happens with avr-gcc (GCC) 7.3.0

/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c: In function ‘test_libc_swprintf’:
/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c:42:5: error: format not a string literal and no format arguments [-Werror=format-security]
     res = swprintf(&sw, "Hello World!");
     ^~~
/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c:49:5: error: format not a string literal and no format arguments [-Werror=format-security]
     res = swprintf(&sw, "0123456789");
     ^~~
/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c:54:5: error: format not a string literal and no format arguments [-Werror=format-security]
     res = swprintf(&sw, "01234567891");
     ^~~
/home/benpicco/dev/RIOT/tests/unittests/tests-libc/tests-libc.c:59:5: error: format not a string literal and no format arguments [-Werror=format-security]
     res = swprintf(&sw, "###");
     ^~~

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think it's because the TO_FLASH() is interfering

derMihai
derMihai reviewed Sep 16, 2024
View reviewed changes
derMihai
derMihai reviewed Sep 16, 2024
View reviewed changes
@benpicco benpicco added this pull request to the merge queue Dec 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@benpicco benpicco added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@benpicco benpicco requested a review from nandojve as a code owner December 17, 2024 13:00
@github-actions github-actions bot added the Area: cpu Area: CPU/MCU ports label Dec 17, 2024
@benpicco benpicco added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@benpicco benpicco added this pull request to the merge queue Dec 20, 2024
Merged via the queue into RIOT-OS:master with commit 243ca31 Dec 20, 2024
@benpicco benpicco deleted the string_writer branch December 21, 2024 10:05
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie left review comments

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@maribu maribu Awaiting requested review from maribu

@fabian18 fabian18 Awaiting requested review from fabian18

@nandojve nandojve Awaiting requested review from nandojve nandojve is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

+1 more reviewer

@derMihai derMihai derMihai left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2025.01

Development

Successfully merging this pull request may close these issues.

7 participants

@benpicco @maribu @Teufelchen1 @riot-ci @aabadie @derMihai @MrKevinWeiss