search

apache / nuttx-apps

Apache NuttX Apps is a collection of tools, shells, network utilities, libraries, interpreters and can be used with the NuttX RTOS
https://nuttx.apache.org/
Apache License 2.0
270 stars 512 forks source link

Requesting justification for bad code pattern #1643

Closed slorquet closed 1 year ago

slorquet commented 1 year ago

Hello, What is the exact reason for this wasteful array that could be replaced by a sprintf?

https://github.com/apache/nuttx/blob/master/libs/libc/string/lib_strsignal.c#L37

Thank you.

pkarashchenko commented 1 year ago

I think with sprintf you can't make strsignal reentrant.

slorquet commented 1 year ago

Is there really no other way to implement this? It looks particularly inefficient.

pkarashchenko commented 1 year ago

Not that I'm aware of. You can see that other cases in switch/case also have a common SIG part of the string. The only difference is that in the array it is a bit longer. Let me think a bit of what can be an option. Maybe having a reentrant variant can be a config option or we can return just the number string and drop Signal prefix

pkarashchenko commented 1 year ago

@slorquet this issue in not nuts-apps related, but anyway I spent some time looking into specs and tinkering some improvements in https://github.com/apache/nuttx/pull/8867. Tests are still in progress and I would appreciate if you can provide a testing feedback from your end as well (despite changes seems to be trivial, I will not be able to test it this week).

pkarashchenko commented 1 year ago

@slorquet can we close this issue now?

slorquet commented 1 year ago

we can :)