Skip to content

C++ Warning fixes, werror on ci, C++17 compliance#2957

Merged
Wumpf merged 22 commits intomainfrom
andreas/cpp/warning-fixes-and-ci-werror
Aug 15, 2023
Merged

C++ Warning fixes, werror on ci, C++17 compliance#2957
Wumpf merged 22 commits intomainfrom
andreas/cpp/warning-fixes-and-ci-werror

Conversation

@Wumpf
Copy link
Copy Markdown
Member

@Wumpf Wumpf commented Aug 10, 2023 •
edited by github-actions bot
Loading

What

  • Fixes Build with warnings as errors on CI for C++ #2903
  • Part of C++ SDK V1 #2919
  • currate warning list for gcc/clang
  • enable warnings as errors on CI, for simplicity it's always active on roundtrip tests
  • enable standard set of warning settings on all C++ targets
  • fix warnings in codegen'ed code and manually written code
  • fix accidental C++20 (should have been caught in theory by warnings but it didn't trigger for unknown reason)

Checklist

@Wumpf Wumpf added 🧑‍💻 dev experience developer experience (excluding CI) sdk-cpp C/C++ API specific labels Aug 10, 2023
@Wumpf Wumpf force-pushed the andreas/cpp/warning-fixes-and-ci-werror branch from c251103 to 565f760 Compare August 10, 2023 16:44
@emilk
Copy link
Copy Markdown
Member

emilk commented Aug 10, 2023

🤨

image

@emilk
Copy link
Copy Markdown
Member

emilk commented Aug 10, 2023

fix accidental C++20 (should have been caught in theory by warnings but it didn't trigger for unknown reason)

Try -Wc99-extensions

emilk
emilk reviewed Aug 10, 2023
View reviewed changes
@Wumpf
Copy link
Copy Markdown
Member Author

Wumpf commented Aug 11, 2023 •
edited
Loading

-Wc99-extensions sadly still didn't catch

        rr_data_row c_data_row {
            .entity_path = entity_path,
            .num_instances = static_cast<uint32_t>(num_instances),
            .num_data_cells = static_cast<uint32_t>(num_data_cells),
            .data_cells = c_data_cells.get(),
        };

but left it in anyways

@Wumpf Wumpf requested a review from emilk August 11, 2023 07:55
@Wumpf Wumpf mentioned this pull request Aug 11, 2023
Merged
3 tasks
@Wumpf
Copy link
Copy Markdown
Member Author

Wumpf commented Aug 11, 2023

Remaining warning happens to be fixed in #2959. Not duplicating it in here, waiting for that instead!

emilk
emilk reviewed Aug 11, 2023
View reviewed changes
Wumpf added a commit that referenced this pull request Aug 11, 2023
@Wumpf
C++ MSVC compilation fixes (#2959)
aa38913 
### What

As kindly supplied by a user on Discord. Didn't get to test them myself
yet but makes lot of sense to me regardless.

* Part of #2919
* requires additionally C++20 fixes from #2957
* currently every dependend binary has to know about arrow, should be
fixed as part of #2873

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2959) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/2959)
- [Docs
preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Fwindows-fixes/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Aandreas%2Fcpp%2Fwindows-fixes/examples)
@Wumpf Wumpf force-pushed the andreas/cpp/warning-fixes-and-ci-werror branch from 312d517 to 9c1db7b Compare August 11, 2023 14:58
emilk
emilk approved these changes Aug 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Always nice to run on as many warnings as possible, especially for unsafe languages

Wumpf added 16 commits August 15, 2023 10:14
@Wumpf
handle call enum cases on switch case in union copy constructor
5850759
@Wumpf
fix switch thing for real TODO REBASE
ee503ba
@Wumpf
fixed signedness and unused variable/typedef bugs
9dcd000
@Wumpf
currated warning lists better - removed redundant and added compatibi…
ea19446 
…lity flags
@Wumpf
roundtrip tests and example builds now configure cmake with warnings …
b52aaf1 
…as errors enabled
@Wumpf
more signedness fixes
3191ab7
@Wumpf
activate warnings for examples and tests
8a70bc7
@Wumpf
fix use of C++20 syntax
9f18645
@Wumpf
werror option on all cpp build scripts
df4c5fa
@Wumpf
enable clang compatible warnings only on clang
61af0b8
@Wumpf
enable warnings on c99 extensions
dd0eef0
@Wumpf
fail shell scripts on unknown argument
e91f818
@Wumpf
c99-extensions only works on clang
9149f09
@Wumpf
workaround old cmake not knowing CMAKE_COMPILE_WARNING_AS_ERROR
5b15a2d
@Wumpf
workaround memcpy warning on gcc
a57d4ad
@Wumpf
more memcpy workarounds
ffa6b64
@Wumpf Wumpf force-pushed the andreas/cpp/warning-fixes-and-ci-werror branch from 9c1db7b to b166661 Compare August 15, 2023 08:19
@Wumpf Wumpf merged commit 16635be into main Aug 15, 2023
@Wumpf Wumpf deleted the andreas/cpp/warning-fixes-and-ci-werror branch August 15, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilk emilk emilk approved these changes

Assignees

No one assigned

Labels

🧑‍💻 dev experience developer experience (excluding CI) sdk-cpp C/C++ API specific

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Build with warnings as errors on CI for C++

2 participants

@Wumpf @emilk