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

1201: Add -Werror for CI builds that are warning clean #1209

Merged
merged 14 commits into from
Jan 13, 2021

Conversation

cz4rs
Copy link
Contributor

@cz4rs cz4rs commented Jan 8, 2021

fixes #1201

@cz4rs
Copy link
Contributor Author

cz4rs commented Jan 8, 2021

I was overly optimistic with this because I was using Debug configuration in local (which passes happily)

I will take a quick look at how much work it would take to make basic gcc and clang builds to pass

@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from 4998969 to 5b465d9 Compare January 8, 2021 15:01
@PhilMiller
Copy link
Member

There's a funny quirk of compiler diagnostics, that some of them only become apparent with optimizations enabled, because the analysis that makes the need for a diagnostic apparent is part of an optimization pass. Those passes are skipped with optimizations disabled for the sake of performance.

@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from 5b465d9 to 702b5cd Compare January 8, 2021 23:59
@codecov
Copy link

codecov bot commented Jan 9, 2021

Codecov Report

Merging #1209 (ce570a7) into develop (e87c3f9) will increase coverage by 0.00%.
The diff coverage is 86.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1209   +/-   ##
========================================
  Coverage    80.99%   81.00%           
========================================
  Files          730      730           
  Lines        28046    28044    -2     
========================================
  Hits         22716    22716           
+ Misses        5330     5328    -2     
Impacted Files Coverage Δ
src/vt/messaging/active.cc 84.30% <ø> (ø)
src/vt/runtime/component/diagnostic_value.h 88.34% <ø> (ø)
...unit/sequencer/test_sequencer_parallel.extended.cc 100.00% <ø> (ø)
.../vt/vrt/collection/balance/stats_restart_reader.cc 82.55% <81.81%> (ø)
tests/unit/collection/test_lbstats_retention.cc 100.00% <100.00%> (ø)
tests/unit/diagnostics/test_diagnostic_value.cc 95.65% <100.00%> (ø)
src/vt/runtime/runtime_diagnostics.cc 0.00% <0.00%> (ø)
src/vt/configs/error/pretty_print_message.cc 3.33% <0.00%> (+0.10%) ⬆️

@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch 4 times, most recently from 91e17f6 to aef521b Compare January 11, 2021 23:20
src/vt/messaging/send_info.h Outdated Show resolved Hide resolved
@cz4rs
Copy link
Contributor Author

cz4rs commented Jan 11, 2021

note to self:

  • update documentation (build section) with new configuration variables
  • make sure that builds fail as expected when a warning is introduced

@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from e651f3d to 2bfe0b4 Compare January 12, 2021 00:06
@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from 2bfe0b4 to 9508eb9 Compare January 12, 2021 11:36
@cz4rs cz4rs marked this pull request as ready for review January 12, 2021 15:18
@cz4rs
Copy link
Contributor Author

cz4rs commented Jan 12, 2021

I have fixed a couple of simple warnings which allowed to enable -Werror for all builds except:

  • nvcc builds (it will not fail even when warnings are present)
  • extended builds
  • gcc-5 - raises defined but not used for the code which uses #pragma GCC diagnostic ignored "-Wunused-variable"
  • gcc-8 - asan build, complains about vt::runtime::component::detail::DiagnosticValueWrapper members being potentially used uninitialized ([-Werror=maybe-uninitialized])

@cz4rs
Copy link
Contributor Author

cz4rs commented Jan 12, 2021

note: this is disabled by default, enabled only for selected builds - up for debate if we should turn this around

@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from 4fea380 to 5dcbcbd Compare January 12, 2021 23:36
@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from 158254d to 5253f7f Compare January 12, 2021 23:51
@lifflander
Copy link
Collaborator

  • DiagnosticValueWrapper

For DiagnosticValueWrapper, we should change this line T initial_value_; to T initial_value_ = {};

Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I think the default compiler should probably be changed back unless there was a reason.

.env Outdated Show resolved Hide resolved
- add -Wno-missing-braces flag for clang++ versions older than 6.0
- revert "#1201: fix warning: use double braces for initialization"
  (commit 864d52b)
- revert "#1201: fix warning: use double braces for initialization"
  (commit c251088)
cz4rs added 2 commits January 13, 2021 18:53
pragma GCC diagnostic ignored "-Wunused-variable" does not work with
this version of the compiler.
@cz4rs cz4rs force-pushed the 1201-treat-warnings-as-errors branch from d5f28aa to ce570a7 Compare January 13, 2021 18:01
@cz4rs
Copy link
Contributor Author

cz4rs commented Jan 13, 2021

Copy link
Contributor

@jstrzebonski jstrzebonski left a comment

Choose a reason for hiding this comment

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

Looks good to me

@cz4rs cz4rs merged commit 43da34d into develop Jan 13, 2021
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.

Add -Werror for CI builds that are warning clean
5 participants