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

style: Replace use of Variable Length Arrays in C code #3323

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Jan 2, 2024

Current C language support for GRASS is C11 with core features, which does not include Variable Length Arrays (VLA). There are, however, a few places in the code that make use of VLAs. This leads us to either update the support directives; insert conditional code (using __STDC_NO_VLA__) ; or replace the use of such code.
Given the limited range of changes to the code, I will with this PR suggest to remove the use of VLA and make the code adhere to the RFC 7.

In addition to the code fixes, the -Wvla CFLAG is added to the Mac and Ubuntu CI builds to prevent VLA code to slip in again.

@nilason nilason added this to the 8.4.0 milestone Jan 2, 2024
@nilason nilason self-assigned this Jan 2, 2024
@github-actions github-actions bot added CI Continuous integration raster Related to raster data processing C Related code is in C libraries module labels Jan 2, 2024
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Oh, they were mandatory in C99, but are optional in C11. Well, we can do without them.

lib/imagery/iscatt_core.c Show resolved Hide resolved
lib/imagery/iscatt_core.c Outdated Show resolved Hide resolved
@nilason
Copy link
Contributor Author

nilason commented Jan 2, 2024

Oh, they were mandatory in C99, but are optional in C11. Well, we can do without them.

This is really not any real-life problem: some of them were introduced already in the early 2010s. But we have no actual benefit for using them either. For comparison, the use of VLAs were deliberately and actively removed from the Linux kernel, for security and efficiency reasons.

@nilason
Copy link
Contributor Author

nilason commented Jan 2, 2024

And, I might add, VLAs will still be optional for also for C23. It was never really a popular addition to the standard.

@echoix
Copy link
Member

echoix commented Jan 2, 2024

For me, it's the first time we see the auto-labeller working ;)

I was wondering about what should the imagery code be considered. Is it at the same level as raster and vector? Here, the labeller added libraries , and module for the files in top level imagery and inside lib/imagery.
Should imagery be a label itself, even if module matches a large part of our code?

@nilason
Copy link
Contributor Author

nilason commented Jan 2, 2024

For me, it's the first time we see the auto-labeller working ;)

I was wondering about what should the imagery code be considered. Is it at the same level as raster and vector?

Yes, along with eg. raster3d too.

Here, the labeller added libraries , and module for the files in top level imagery and inside lib/imagery. Should imagery be a label itself, even if module matches a large part of our code?

There is imagery code both in libraries as well as in modules, so this is working as intended. A proper label description might be added though and some m&m colour applied.

@echoix
Copy link
Member

echoix commented Jan 2, 2024

For me, it's the first time we see the auto-labeller working ;)

I was wondering about what should the imagery code be considered. Is it at the same level as raster and vector?

Yes, along with eg. raster3d too.

So it's missing labels for these. ;)

Here, the labeller added libraries , and module for the files in top level imagery and inside lib/imagery. Should imagery be a label itself, even if module matches a large part of our code?

There is imagery code both in libraries as well as in modules, so this is working as intended. A proper label description might be added though and some m&m colour applied.

Yep, the libraries and module match correctly.
Colors, I'll check it out soon.

@nilason nilason merged commit e6a3771 into OSGeo:main Jan 8, 2024
24 checks passed
@nilason
Copy link
Contributor Author

nilason commented Jan 8, 2024

Thanks @wenzeslaus !

HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request Jan 9, 2024
* CI: add -Wvla
* Replace use of Variable Length Arrays
* use sizeof(int)
@nilason nilason deleted the remove_vla_code branch February 17, 2024 20:07
@wenzeslaus wenzeslaus changed the title Replace use of Variable Length Arrays in C code style: Replace use of Variable Length Arrays in C code Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C CI Continuous integration libraries module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants