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

Use -Werror and -Wpedantic in CI #238

Open
wants to merge 3 commits into
base: workflow
Choose a base branch
from

Conversation

frankplow
Copy link
Collaborator

This patchset does two things:

  • Adds the -Werror flag for only the libavcodec/vvc/*.o objects. This turns any compilation warnings for these files into CI failures.
  • Adds the -Wpedantic flag, which adds warnings whenever non-standard C features are used. This should help catch errors such as that fixed by FFmpeg/FFmpeg@75e3b81

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

@frankplow
Copy link
Collaborator Author

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

@nuomi2021
Copy link
Member

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

@frankplow
Copy link
Collaborator Author

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

Unfortunately there does not appear to be any easy way to do this. There are no command line flags for GCC to selectively disable some ISO compliance checks, either it tests if the code is ISO C compliant or not. The only workaround would be to use pragmas to selectively disable -Wpedantic for only the problematic lines, but we'd have to patch this in and the diff would break with nearly any change to the code so I don't think this is workable. I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

@nuomi2021
Copy link
Member

nuomi2021 commented Jun 30, 2024

I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

If this costs too much time, it may not be worth it. Focus on fuzz may help us more.

But if you want, we can change

pixel sel[VVC_MAX_SAMPLE_ARRAYS][MAX_PICK_POS * 2];
to 1d array like pixel sel[VVC_MAX_SAMPLE_ARRAYS * MAX_PICK_POS * 2];

Thank you

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.

2 participants