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

CI: Fail QA on warnings by clang-14 and gcc-11 #27

Merged
merged 6 commits into from
Nov 7, 2022

Conversation

jtxa
Copy link
Contributor

@jtxa jtxa commented Nov 6, 2022

Fixed most warnings for -std=c++11 -pedantic -Wall -Wextra.
Disabled only -Wno-implicit-fallthrough on gcc for now.
info for older gcc, -pedantic may throw more messages: gcc-8 doesn't like the extra ; on the end of namespace.

Added a gcc-11 build to QA workflow, enabled -Werror for it and clang. Added -k 0 to ninja call, to keep it compiling all files.

  • C99 VLA: I replaced the VLA by new/delete combination, as I have seen this as the solution in other files.
  • Unused argument: removing the named argument is the solution in other files.
  • write_data: I don't know if these functions have any relation. But renaming it was the easiest solution here. And write_record_data matches quite well the function directly about it write_record_header.

Contrary to C++17 a message is needed for C++11.

clang++ warning:

~~~
static_assert with no message is a C++17 extension [-Wc++17-extensions]
~~~
@jtxa jtxa marked this pull request as ready for review November 6, 2022 18:57
@jtxa jtxa mentioned this pull request Nov 6, 2022
9 tasks
jtxa added 5 commits November 6, 2022 22:45
VLA is a feature of C99 which is not available in C++11 or even later.
Avoid using this non-standard extension.

clang++ warning:

~~~
variable length arrays are a C99 feature [-Wvla-extension]
~~~
The function `starts_with` is only used inside `assert` calls.
In case of release builds it is unused.

Add a preprocessor condition to activate it only when needed.

g++ warning:

~~~
warning: ‘bool starts_with(const string&, const string&)’ defined but not used [-Wunused-function]
~~~
clang++ warning:

~~~
note: hidden overloaded virtual function 'srecord::output::write_data' declared here: different number of parameters (3 vs 1)
~~~
clang++ warning:

~~~
warning: unused parameter 'cmdln' [-Wunused-parameter]
~~~
Do a build with `-std=c++11 -pedantic -Wall -Wextra` enabled for
clang and gcc.

Use `-Werror` to abort on any warning.
But let build keep going by calling `ninja -k 0` and not abort on the
first file with errors.
@jtxa jtxa force-pushed the ci-abort-on-warnings branch from 0660c10 to 66e7d44 Compare November 6, 2022 21:50
@jtxa
Copy link
Contributor Author

jtxa commented Nov 6, 2022

Moved new/delete out of for-loop. Force pushed.

@jtxa
Copy link
Contributor Author

jtxa commented Nov 6, 2022

I assume that all new/delete call in SRecord may be replaced by something more modern; smart pointers and their helper functions or so. At least RAII shall be used, RAII may also be useful elsewhere (I haven't looked how files are opened/closed).

But I did not invest any time on that for now, because this change should be simple & straight forward so we can enforce warning free check soon.

@sierrafoxtrot
Copy link
Owner

But I did not invest any time on that for now, because this change should be simple & straight forward so we can enforce warning free check soon.

Totally agree with this. Happy to look into RAII and other more modern approaches if there is a solid reason to rewrite. Let's chat!

@sierrafoxtrot
Copy link
Owner

I stupidly hit close (facepalm). Merging now

@sierrafoxtrot sierrafoxtrot merged commit 7e7d93f into sierrafoxtrot:master Nov 7, 2022
@jtxa jtxa deleted the ci-abort-on-warnings branch November 7, 2022 17:28
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