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

Parser: don't issue a bounds warning for trailing one-element array access #330

Merged
merged 3 commits into from
Jul 9, 2022

Conversation

ehaas
Copy link
Collaborator

@ehaas ehaas commented Jul 3, 2022

Closes #327

@ehaas
Copy link
Collaborator Author

ehaas commented Jul 3, 2022

After this change we're down to 19 warnings in sqlite.

2 from overflow due to incorrect sizeof calculations
2 from #328
4 from #325
11 from unreachable code warnings

@ehaas ehaas force-pushed the old-style-flexible-array branch from 791dc08 to 66c8259 Compare July 3, 2022 16:22
@Vexu
Copy link
Owner

Vexu commented Jul 4, 2022

This does seem to work with everything I try but it feels a bit odd having it be part of the global state of the Parser. Is there some specific reason why you chose this approach instead of something more confined in checkArrayBounds?

@ehaas
Copy link
Collaborator Author

ehaas commented Jul 5, 2022

I'd prefer to do it in checkArrayBounds but I couldn't think of a way to tell if I was dealing with a trailing one-element array field. GCC doesn't seem to do any bounds checking at all; clang will issue a warning if it's a standalone array or non-trailing one-element field, but no warning if it's a trailing one element field.

@Vexu
Copy link
Owner

Vexu commented Jul 5, 2022

Seems to me that if you pass checkArrayBounds the array Result instead of just the type, then you could inline Parser.getNode and move the logic you currently have in fieldAccessExtra there.

@Vexu Vexu force-pushed the old-style-flexible-array branch from adef4fc to 66c8259 Compare July 5, 2022 09:11
@Vexu
Copy link
Owner

Vexu commented Jul 5, 2022

Accidentally pushed unrelated changes to this branch, reverted now.

@InKryption
Copy link

Is there/will there be an option to enable a warning for this?

@ehaas
Copy link
Collaborator Author

ehaas commented Jul 5, 2022

Is there/will there be an option to enable a warning for this?

I don't think Clang has an option to enable a warning for this (didn't see any here https://clang.llvm.org/docs/DiagnosticsReference.html) so we could create our own. Any suggestions for the name?

@Vexu
Copy link
Owner

Vexu commented Jul 5, 2022

I couldn't find anything either, my suggestion is -Wold-style-flexible-struct default warning with the same message as .array_after.

@ehaas ehaas force-pushed the old-style-flexible-array branch from 66c8259 to ad492ae Compare July 6, 2022 03:52
@ehaas
Copy link
Collaborator Author

ehaas commented Jul 6, 2022

Updated to keep the check local to checkArrayBounds, removed bounds checking for zero-element arrays, added a warning for zero-element arrays (at creation time) and added the -Wold-style-flexible-struct warning. I set it to default off since clang doesn't issue that warning by default but I can switch it to warn by default if you want.

@Vexu
Copy link
Owner

Vexu commented Jul 6, 2022

I'm fine with default off but it should be part of all, extra, or pedantic. Whichever you deem best fit.

@Vexu Vexu force-pushed the old-style-flexible-array branch from a876fef to e655bba Compare July 9, 2022 08:12
@Vexu Vexu merged commit b3446b1 into Vexu:master Jul 9, 2022
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.

Alternative syntax for flexible arrays
3 participants