Skip to content

Fix build warnings#1613

Merged
esigo merged 4 commits into
open-telemetry:mainfrom
marcalff:fix_build_warnings_1611
Sep 14, 2022
Merged

Fix build warnings#1613
esigo merged 4 commits into
open-telemetry:mainfrom
marcalff:fix_build_warnings_1611

Conversation

@marcalff

Copy link
Copy Markdown
Member

Fixes #1611

Changes

Fix build warnings reported by gcc 10, with:
-Wall -Werror
-Wextra-semi
-Werror=unused-parameter
-Werror=undef

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Fix build warnings reported by gcc 10, with:
 -Wall -Werror
 -Wextra-semi
 -Werror=unused-parameter
 -Werror=undef
@marcalff marcalff requested a review from a team September 13, 2022 22:07
@marcalff marcalff changed the title Fix build warnings issue #1611 Fix build warnings Sep 13, 2022
Comment thread exporters/otlp/test/otlp_http_metric_exporter_test.cc Outdated
@codecov

codecov Bot commented Sep 13, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1613 (1c83594) into main (74f0ac1) will decrease coverage by 0.03%.
The diff coverage is 70.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
- Coverage   85.24%   85.22%   -0.02%     
==========================================
  Files         156      156              
  Lines        4978     4978              
==========================================
- Hits         4243     4242       -1     
- Misses        735      736       +1     
Impacted Files Coverage Δ
...elemetry/context/propagation/text_map_propagator.h 50.00% <0.00%> (ø)
api/include/opentelemetry/nostd/string_view.h 98.04% <ø> (ø)
api/include/opentelemetry/trace/noop.h 93.11% <ø> (ø)
...ude/opentelemetry/trace/span_context_kv_iterable.h 100.00% <ø> (ø)
...nclude/opentelemetry/ext/http/client/http_client.h 93.34% <ø> (ø)
ext/src/http/client/curl/http_operation_curl.cc 76.03% <ø> (ø)
...ry/sdk/metrics/aggregation/histogram_aggregation.h 90.00% <0.00%> (ø)
...ry/sdk/metrics/aggregation/lastvalue_aggregation.h 0.00% <0.00%> (ø)
...elemetry/sdk/metrics/aggregation/sum_aggregation.h 0.00% <0.00%> (ø)
...telemetry/sdk/metrics/state/async_metric_storage.h 85.30% <ø> (ø)
... and 23 more

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, but as it's bigger cleanup, will wait for one more approval :). Thanks for taking this.

Comment thread sdk/src/metrics/state/temporal_metric_storage.cc Outdated
Comment thread sdk/src/metrics/state/temporal_metric_storage.cc Outdated
remove debug code

@owent owent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after the dead codes removed.

@marcalff

Copy link
Copy Markdown
Member Author

The DocFX check build failed due to this:

The install of docfx was NOT successful.
docfx not installed. An error occurred during installation:
 The remote server returned an error: (500) Internal Server Error. Internal Server Error

which looks like a transient error unrelated to the change.

@lalitb , @ThomsonTan

This PR is ready for merge now.

@esigo esigo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM
Thanks for the fix :)

@esigo

esigo commented Sep 14, 2022

Copy link
Copy Markdown
Member

related to #249

@esigo esigo merged commit c7dfc0d into open-telemetry:main Sep 14, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
@marcalff marcalff deleted the fix_build_warnings_1611 branch July 4, 2023 07:10
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.

[BUILD] Build warnings

5 participants