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

Fix clang vla problems #3281

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

BsAtHome
Copy link
Contributor

Variable length arrays (vla) are a feature of C and are not in the C++ standard. Both gcc and clang have fixes for this, but they remain non-standard and result in a warning in clang.

This PR removes the use of vla and replaces it with new[]/delete[]. This is all non-RT code, so that is no problem. In the process it uncovered some subtle and less subtle problems in the code:

  • strtok_r() reused the state pointer as string argument. This is very inappropriate because the state pointer is an opaque libc internal state. You should never depend that. The code is modified to use documented strtok_r() procedures.
  • Several instances did not check the vla size for being zero or negative. Both cases are not handled well in vla's. The code is modified to test for negative or zero lengths and acts accordingly.

Remove variable length array (unsupported in C++).
Handle case when no components are found.
…ng argument.

Remove variable length array (unsupported in C++).
Add safeguard against zero (or negative) length array allocation.
@rmu75
Copy link
Contributor

rmu75 commented Jan 13, 2025

IMO in 2025 it is a bug to use naked "new" in C++. Isn't it possible to use std::unique_ptr and std::make_unique? something like auto buf = make_unique<hal_stream_data[]>(n);. C++17 should be OK. Or even use a std::vector.

@BsAtHome
Copy link
Contributor Author

You are probably right. However std::vector is unlikely to work considering the way the rest of the code works. I will look into the alternatives.

@rmu75
Copy link
Contributor

rmu75 commented Jan 14, 2025

std::vector always allocates a contiguous array, it is kind of auto-resizing dynamic array. when constructing with size it should do only one allocation. So it should be possible to get the pointer to first element and leave the code more or less unchanged. I will take a look.

Better solution would be something like dynamic std::array, but that doesn't seem to exist in standard C++. Maybe std::valarray? Sometime in the future.

We should put comment there why it's done like that, sometime in the future either val arrays will come to C++ or prober stack-allocated std::array with at-construction size (vs. at-compile size as it is now).

@BsAtHome
Copy link
Contributor Author

std::vector always allocates a contiguous array,...

Yes, but I was thinking about string handling, not generic arrays. There you need the terminating NUL for C-string compatibility. Luckily, the string class keeps the array NUL terminated since C++11.

Anyway, it should now be resolved with the commit. Please review.

@rmu75
Copy link
Contributor

rmu75 commented Jan 14, 2025

I like that much better, LGTM.

@andypugh andypugh merged commit ba7542e into LinuxCNC:master Jan 15, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants