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

April 2024 warning police #4297

Merged
merged 18 commits into from
May 7, 2024
Merged

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Apr 26, 2024

Fix some warnings that cropped up from (a) recent work in overlay and buckets and (b) recent upgrades to my copy of clangd which now has IWYU turned on by default, and I figured .. why not start listening to it and cleaning things up incrementally?

@@ -5862,7 +5862,7 @@ TEST_CASE("SCP message capture from previous ledger", "[herder]")
// Initialize simulation
auto networkID = sha256(getTestConfig().NETWORK_PASSPHRASE);
auto simulation = std::make_shared<Simulation>(
Simulation::OVER_LOOPBACK, networkID, [version](int i) {
Simulation::OVER_LOOPBACK, networkID, [](int i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless Microsoft fixed their compiler this unnecessary version capture is necessary due to an MSVC bug (see #4194, and specifically this comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed (crudely: made it non-constexpr and captured it)

@bboston7
Copy link
Contributor

If this change resolves all of the warnings, I also wonder what your thoughts are on setting -Werror in CI to ensure future warnings don't slip in.

@graydon
Copy link
Contributor Author

graydon commented Apr 26, 2024

If this change resolves all of the warnings, I also wonder what your thoughts are on setting -Werror in CI to ensure future warnings don't slip in.

It doesn't. there are warnings in several submodules and depending on the compiler and compiler version you use you'll get a different set. In general while I appreciate the moral goal of -Werror, I think in practice the C++ compiler ecosystem and language stability model makes it .. not very viable most of the time. It's more viable in Rust! But even then people (cough cough -- us) sometimes turn on "error on any clippy warning" and then a single github actions builder-image update breaks the build.

Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

LGTM! It looks like we're not compiling though in CI, I think you might have removed a few too many includes.

@bboston7
Copy link
Contributor

It doesn't. there are warnings in several submodules and depending on the compiler and compiler version you use you'll get a different set. In general while I appreciate the moral goal of -Werror, I think in practice the C++ compiler ecosystem and language stability model makes it .. not very viable most of the time. It's more viable in Rust! But even then people (cough cough -- us) sometimes turn on "error on any clippy warning" and then a single github actions builder-image update breaks the build.

Makes sense, thanks!

@graydon
Copy link
Contributor Author

graydon commented Apr 29, 2024

LGTM! It looks like we're not compiling though in CI, I think you might have removed a few too many includes.

This is weird! I hate to be the "it works on my machine" guy but .. it works on my machine! I will try to figure out the cause.


// Initialize simulation
auto networkID = sha256(getTestConfig().NETWORK_PASSPHRASE);
auto simulation = std::make_shared<Simulation>(
Simulation::OVER_LOOPBACK, networkID, [](int i) {
Simulation::OVER_LOOPBACK, networkID, [version](int i) {
auto cfg = getTestConfig(i, Config::TESTDB_ON_DISK_SQLITE);
cfg.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION = version;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed at all? I think this test should just use the most recent version for testing, which is the default, if we don't override TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why the test is hard-wired to a single version. possibly copypasta. it works fine without that line and in fact without the ConfigGen argument at all.

@graydon graydon force-pushed the april-2024-warning-police branch from 9347c5b to 142f833 Compare May 2, 2024 08:11
@graydon graydon force-pushed the april-2024-warning-police branch from 142f833 to 080ee6d Compare May 2, 2024 08:29
@graydon
Copy link
Contributor Author

graydon commented May 2, 2024

I will try to figure out the cause

It was too embarrassingly silly to mention here. Anyway, fixed.

@marta-lokhova
Copy link
Contributor

r+ efb9d1e

@latobarita latobarita merged commit c6f4741 into stellar:master May 7, 2024
15 checks passed
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.

6 participants