Skip to content
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

Name pointers should be constant #61

Open
josesimoes opened this issue Dec 15, 2020 · 7 comments
Open

Name pointers should be constant #61

josesimoes opened this issue Dec 15, 2020 · 7 comments
Assignees
Labels
hardware New hardware or architecture support request

Comments

@josesimoes
Copy link

josesimoes commented Dec 15, 2020

On every call that has a name in the parameters e.g. tx_byte_pool_create, tx_timer_create, etc the name_ptr should be const CHAR *name_ptr instead of the current CHAR *name_ptr.

It's not only a GP to give that hint to the compiler but also makes the code usable in C++ without having to add a cast on each of those names. (ISO C++ forbids converting a string constant to 'CHAR*').

If you want it, I have a PR ready with this fix.

@hwmaier
Copy link

hwmaier commented Dec 15, 2020

Very good point. I support this as well. Having those C strings non const is a major nuisance, in particular for C++ users. Our code is cluttered with unnecessary casts and #pragma GCC diagnostic ignored "-Wwrite-strings" statements to make the code compile with C++.

@goldscott goldscott self-assigned this Dec 17, 2020
@goldscott
Copy link
Contributor

Hi All,

We on the RTOS team agree with you - we'd like to make this const. Unfortunately, we can not change the existing API. This change requires lots of documentation work (API definitions/example code/etc.), coding, and testing. This also goes well beyond just ThreadX – all components and add-on protocols would need the same. ThreadX and most of our middleware is going through the re-certification process and this change can't be made right now.

(const) Sorry.

@josesimoes
Copy link
Author

hey @goldscott

Thanks for the quick response.
And I understand the certification part and that you can't make this change right now.

As for the rest, let me stress that this only involves updating the documentation on the API declaration (apart from the code, of course). No changes are required in the samples, example code and explanations. In the end this acts a compiler hint and letting the caller know that the callee wont change the content of the string.

Plus this is not a breaking change in the sense that the existing code has to be modified to use the new declaration.

Last thought: I understand that developers want stability (and I count myself among those) but that should never prevent improving things and moving forward. Having said that one can always provide alternative APIs and mark others as obsolete and plan for their removal in X versions.

Please give it another tough or, at least, put it on the backlog for the spring cleaning.

@yuxin-azrtos
Copy link
Contributor

@josesimoes we agree with you that we will review this in the future. Close the issue now. Feel free to reopen it or reach out to us if you have additional thoughts or comments. Thanks!

@yuxin-azrtos yuxin-azrtos added the hardware New hardware or architecture support request label Feb 8, 2021
@csrichter
Copy link

any updates on when this issue will be worked on?

as @josesimoes said, this fix should be backwards compatible with all existing code and examples. It shouldn't break the API for existing users

@goldscott
Copy link
Contributor

Hi @csrichter @josesimoes @hwmaier - we are working on this now!

@hwmaier
Copy link

hwmaier commented Oct 2, 2024

Thankfully we have now a PR to address this long standing issue. Refer to #414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hardware New hardware or architecture support request
Projects
None yet
Development

No branches or pull requests

5 participants