aws / aws-sdk-cpp

AWS SDK for C++
Apache License 2.0
1.97k stars 1.06k forks source link

Compilation warning message in aws-crt-cpp #1646

Closed jerrytfleung closed 2 years ago

jerrytfleung commented 3 years ago

Describe the bug A compilation warning message is popped up during building aws-crt-cpp with -Wextra and -Werror compiler options.

[  5%] Building C object aws-sdk-cpp/crt/aws-crt-cpp/crt/aws-c-common/CMakeFiles/aws-c-common.dir/source/encoding.c.o
/Users/runner/work/timestream-odbc/timestream-odbc/src/aws-sdk-cpp/crt/aws-crt-cpp/crt/aws-c-common/source/encoding.c:364:20: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
       if (result == -1) {
           ~~~~~~ ^  ~~
1 error generated.

SDK version number 1.9.19

Platform/OS/Hardware/Device macOS Mojave

To Reproduce (observed behavior)

  1. Add the following line in your cmake project using aws-sdk-cpp 1.9.19
    add_compile_options(-Wall -Wextra -pedantic -Werror)
  2. Build the project and look for the warning message

Expected behavior aws-crt-cpp should be able to build without errors and warnings.

KaibaLopez commented 3 years ago

Hi @jerrytfleung , Thanks for bringing this up to us, we'll take a look, it should be a fairly simple fix. I think it's worth mentioning that most compilers will figure it out and behave the expected way though. this is not to say that the warning is invalid, just that you can safely build without worries while we change it.

KaibaLopez commented 3 years ago

Hi @jerrytfleung , Kind of revisiting this and upon closer inspection, while we could change that one comparison, and it kind of makes sense to do so if only for readability , I don't think we can commit to having the compilation pass the -pedantic flag. It would be a much bigger endeavor and we simply don't have the bandwidth for it. I've already created the PR on aws-crt-cpp for the type comparison change, but I think I'll close this issue as I don't think we can get the whole FR in so I don't think it makes sense to keep it, If you disagree though, feel free to add a comment to keep it open to keep track of when the pr is merged.

jerrytfleung commented 3 years ago

Understand. It is okay to me.

github-actions[bot] commented 2 years ago

Greetings! Sorry to say but this is a very old issue that is probably not getting as much attention as it deservers. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

github-actions[bot] commented 2 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.