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.

Sign up for GitHub

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

Jump to bottom

Compiler warnings clean-up #249

Closed
maxgolov opened this issue Aug 5, 2020 · 10 comments · Fixed by #1677
Closed

Compiler warnings clean-up #249

maxgolov opened this issue Aug 5, 2020 · 10 comments · Fixed by #1677
Assignees
@marcalff
Labels
build and test do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers priority:p2 Issues that are not blocking

Comments

@maxgolov
Copy link
Contributor

maxgolov commented Aug 5, 2020 •
edited
Loading

Repo has 26 compiler warnings with vs2019 in:

  • tracez_processor_test
  • ostream_metrics_test
  • opentelemetry_exporter_ostream_metrics

Most of these are about potentially unsafe truncating type casts:

  • 'initializing': conversion from 'float' to 'int', possible loss of data
  • '=': conversion from 'double' to 'int', possible loss of data
  • '=': conversion from 'size_t' to 'int', possible loss of data
  • 'initializing': conversion from 'size_t' to 'int', possible loss of data
  • 'return': conversion from 'double' to 'T', possible loss of data

Recommendation is to fix those warnings. And change our build flags to All or Level 4 :

Compiler Compiler Flags
gcc -Wall -Wextra
clang -Wall -Wextra
MSVC /W4

Plus treat all warnings as errors by default across all compilers, to keep it at zero warnings at all times. It is gonna be progressively harder to clean it later if we don't keep it at zero from the very beginning.
@maxgolov maxgolov mentioned this issue Sep 3, 2020
Merged
@maxgolov maxgolov added good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers priority:p2 Issues that are not blocking labels Dec 10, 2020
@owent
Copy link
Member

owent commented Apr 2, 2021

Maybe it's better to remove warning from protobuf?
There is a lot warnings which are come from protobuf headers, for example:

[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/stubs/logging.h(102,23): warning C4251: “google::protobuf::internal::LogMessage::message_”: class“std::basic_string<char,std::char_traits<char>,std::allocator<char>>”需要有 dll 接口由 class“google::protobuf::internal::LogMessage”的客户端使用 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29910\include\xstring(4648): message : 参见“std::basic_string<char,std::char_traits<char>,std::allocator<char>>”的声明 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build]   otlp_foo_library.vcxproj -> D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\examples\otlp\Debug\otlp_foo_library.lib
[build]   load_plugin_example.vcxproj -> D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\examples\plugin\load\Debug\load_plugin_example.exe
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/io/coded_stream.h(1250,64): warning C4251: “google::protobuf::io::CodedOutputStream::default_serialization_deterministic_”: struct“std::atomic<bool>”需要有 dll 接口由 class“google::protobuf::io::CodedOutputStream”的客户端使用 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29910\include\atomic(2777): message : 参见“std::atomic<bool>”的声明 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/arena_impl.h(325,37): warning C4251: “google::protobuf::internal::ArenaImpl::threads_”: struct“std::atomic<google::protobuf::internal::SerialArena *>”需要有 dll 接口由 class“google::protobuf::internal::ArenaImpl”的客户端使用 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/arena_impl.h(325): message : 参见“std::atomic<google::protobuf::internal::SerialArena *>”的声明 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/arena_impl.h(326,34): warning C4251: “google::protobuf::internal::ArenaImpl::hint_”: struct“std::atomic<google::protobuf::internal::SerialArena *>”需要有 dll 接口由 class“google::protobuf::internal::ArenaImpl”的客户端使用 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/arena_impl.h(325): message : 参见“std::atomic<google::protobuf::internal::SerialArena *>”的声明 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build] D:\prebuilt\vcpkg\installed\x64-windows\include\google/protobuf/arena_impl.h(327,39): warning C4251: “google::protobuf::internal::ArenaImpl::space_allocated_”: struct“std::atomic<unsigned __int64>”需要有 dll 接口由 class“google::protobuf::internal::ArenaImpl”的客户端使用 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build]     正在创建库 D:/workspace/github/owent-contrib/opentelemetry/opentelemetry-cpp/build_jobs_cmake_tools/examples/plugin/plugin/Debug/example_plugin.lib 和对象 D:/workspace/github/owent-contrib/opentelemetry/opentelemetry-cpp/build_jobs_cmake_tools/examples/plugin/plugin/Debug/example_plugin.exp
[build] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29910\include\atomic(2789): message : 参见“std::atomic<unsigned __int64>”的声明 [D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\opentelemetry_proto.vcxproj]
[build]   dynamic_load_test.vcxproj -> D:\workspace\github\owent-contrib\opentelemetry\opentelemetry-cpp\build_jobs_cmake_tools\api\test\plugin\Debug\dynamic_load_test.exe

Can I create a PR to remove some warnings from protobuf?

@lalitb
Copy link
Member

lalitb commented Apr 2, 2021

@owt5008137 Thanks for reporting these warnings. It would be definitely helpful if you raise PR for fixing these warnings. Please go ahead and raise a PR.

@owent owent mentioned this issue Apr 2, 2021
Merged
3 tasks
@maxgolov
Copy link
Contributor Author

maxgolov commented Apr 6, 2021 •
edited
Loading

@owt5008137 - agree with removing from Protobuf. I like the way how you handled it, esp. push and pop . Thank you!!!

@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@github-actions github-actions bot added the Stale label Nov 26, 2021
@lalitb lalitb removed the Stale label Nov 26, 2021
@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@github-actions github-actions bot added the Stale label Jan 26, 2022
@lalitb lalitb added do-not-stale and removed Stale labels Jan 26, 2022
@github-actions
Copy link

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

@github-actions github-actions bot added the Stale label Mar 27, 2022
@github-actions
Copy link

github-actions bot commented Apr 3, 2022

Closed as inactive. Feel free to reopen if this is still an issue.

@github-actions github-actions bot closed this as completed Apr 3, 2022
@lalitb
Copy link
Member

lalitb commented Apr 3, 2022

Reopening, as we still have warnings.

@lalitb lalitb reopened this Apr 3, 2022
@github-actions github-actions bot removed the Stale label Apr 4, 2022
@ThomsonTan
Copy link
Contributor

@lalitb do we plan to enable /W4 or related options?

@lalitb
Copy link
Member

lalitb commented Apr 12, 2022

@ThomsonTan Yes, we should be enabling this for all platforms once warnings are cleaned-up.

@esigo esigo mentioned this issue Sep 14, 2022
Merged
3 tasks
This was referenced Oct 3, 2022
Closed
Closed
@marcalff marcalff self-assigned this Oct 6, 2022
marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Oct 12, 2022
@marcalff
Fixes open-telemetry#249
7e56cd9 
Fixed many remaining compiler warnings.
@marcalff marcalff mentioned this issue Oct 12, 2022
Merged
3 tasks
esigo pushed a commit that referenced this issue Oct 13, 2022
@marcalff
Fixes #249 (#1677)
f29703b 
Fixed many remaining compiler warnings.
ays7 added a commit to ays7/opentelemetry-cpp that referenced this issue Oct 29, 2022
@ays7
Merge commit '9acde87b08b225ce511fa8a20c6cba14f2921518' into ays/thre…
456a0e7 
…ad-local-stack

* commit '9acde87b08b225ce511fa8a20c6cba14f2921518': (36 commits)
  Prepare v1.7.0 release with Metrics API/SDK GA. (open-telemetry#1721)
  Fix: 1712 -  Validate Instrument meta data (name, unit, description) (open-telemetry#1713)
  Document libthrift 0.12.0 doesn't work with Jaeger exporter (open-telemetry#1714)
  Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter (open-telemetry#1675)
  [Metrics SDK] Move Metrics Exemplar processing behind feature flag (open-telemetry#1710)
  [Metrics API/SDK] Change Meter API/SDK to return nostd::unique_ptr for Sync Instruments (open-telemetry#1707)
  [Metrics] Switch to explicit 64 bit integers (open-telemetry#1686)
  Add metrics e2e test to asan & tsan CI (open-telemetry#1670)
  Add otlp-grpc example bazel (open-telemetry#1708)
  [Metrics SDK] Add support for Pull Metric Reader (open-telemetry#1701)
  Fix debug log of OTLP HTTP exporter and ES log exporter (open-telemetry#1703)
  [SEMANTIC CONVENTIONS] Upgrade to version 1.14.0 (open-telemetry#1697)
  Fix a potential precision loss on integer in ReservoirCellIndexFor (open-telemetry#1696)
  fix Histogram crash (open-telemetry#1685)
  Fix:1676 Segfault when short export period is used for metrics  (open-telemetry#1682)
  Add timeout support to MeterContext::ForceFlush (open-telemetry#1673)
  Add CMake OTELCPP_MAINTAINER_MODE (open-telemetry#1650)
  [DOCS] - Minor updates to OStream Metrics exporter documentation (open-telemetry#1679)
  Fix:open-telemetry#1575 API Documentation for Metrics SDK and API (open-telemetry#1678)
  Fixes open-telemetry#249 (open-telemetry#1677)
  ...
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this issue Dec 5, 2022
@marcalff @yxue
Fixes open-telemetry#249 (open-telemetry#1677)
0b5b781 
Fixed many remaining compiler warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marcalff marcalff

Labels
build and test do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers priority:p2 Issues that are not blocking
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed many remaining compiler warnings marcalff/opentelemetry-cpp
5 participants
@ThomsonTan @lalitb @owent @marcalff @maxgolov