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

Silence warnings during build #84

Merged
merged 2 commits into from
May 10, 2023

Conversation

tamaskenezlego
Copy link
Contributor

@tamaskenezlego tamaskenezlego commented May 1, 2023

The motivation of this PR is to make this project build within our projects since the currently supported method of including snowplow-cpp is to build it as part of the client project. Our projects has different settings which makes certain warnings enabled and turned into errors.

Another solution would be adding an install-target to the CMakeLists.txt. If you're interested, please check the install-cmake-config branch on my fork. It would allow building Snowplow separately.

Changes in this PR:

  • silence std::move-related warnings by remove unneccessary std::move on return values
  • silence integer conversion warnings by applying explicit conversion
  • silence GetVersionEx deprecation warnings with a local #pragma (Windows)
  • fix illegal use of the TEXT macro on non-literal c-strings (Windows)

A comment about the TEXT macro and Windows code pages:

The TEXT macro should only be used on string literals. It does not convert ANSI strings to UTF-16. I fixed this by explicitly switching to the ANSI version of the Windows-API function. Note that using the ANSI versions (which happens also before this PR) is only correct if the strings are ASCII strings or are encoded with the current ANSI code page.

@snowplowcla
Copy link

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@snowplowcla snowplowcla added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label May 1, 2023
@tamaskenezlego
Copy link
Contributor Author

I signed it!

@snowplowcla
Copy link

Confirmed! @tamaskenezlego has signed the Contributor License Agreement. Thanks so much.

@snowplowcla snowplowcla added cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed. and removed cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. labels May 1, 2023
@tamaskenezlego tamaskenezlego marked this pull request as ready for review May 1, 2023 11:16
@matus-tomlein
Copy link
Contributor

Thanks for the contribution, @tamaskenezlego! We will work on reviewing the PR.

@matus-tomlein matus-tomlein changed the base branch from master to release/1.1.0 May 10, 2023 06:35
@matus-tomlein matus-tomlein changed the title Silence warnings Silence warnings during build May 10, 2023
Copy link
Contributor

@matus-tomlein matus-tomlein left a comment

Choose a reason for hiding this comment

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

This looks great, thanks again for the contribution @tamaskenezlego!

@matus-tomlein
Copy link
Contributor

@tamaskenezlego If you could also make a PR with the install-target that you added in the other branch, I think that would be very useful to have in the tracker as well!

@matus-tomlein matus-tomlein merged commit df6be0e into snowplow:release/1.1.0 May 10, 2023
@tamaskenezlego tamaskenezlego deleted the silence_warnings branch May 10, 2023 08:49
@tamaskenezlego
Copy link
Contributor Author

@matus-tomlein , you're welcome! Also, I'll look into the install-target issue and create a PR soon.

tamaskenezlego added a commit to tamaskenezlego/snowplow-cpp-tracker that referenced this pull request May 16, 2023
@tamaskenezlego
Copy link
Contributor Author

@matus-tomlein One question about the headers: all headers are public headers and are to be installed, right?

@tamaskenezlego
Copy link
Contributor Author

@matus-tomlein, one more thing, I'm making changes and wants to use clang-format on my changes. To figure out what clang-format version I need I tried version 14, 15, 16 on master but all three introduced changes everywhere. Are you using an older clang-format, if so, what version?

@matus-tomlein
Copy link
Contributor

Hi @tamaskenezlego, thank you for taking this up!

Currently yes – all headers are public headers and all the source code is in the include folder to make it easier to install by copying the files to user projects.

We used clang-format version 16 with the LLVM style, but don't have this enforced in CI so likely we have diverged from the format over time. But if you can check your changes with clang-format, that'd be great!

@matus-tomlein matus-tomlein mentioned this pull request May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants