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

fix: remove deprecated operator"" syntax usage #661

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

lidavidm
Copy link
Member

Clang warns about this. Apparently GCC 4.9 may warn about the fixed syntax, though!

Spotted in the ADBC CI:

 /adbc/c/vendor/nanoarrow/nanoarrow.hpp:94:35: error: identifier '_asv' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator]
   94 | inline ArrowStringView operator"" _asv(const char* data, std::size_t size_bytes) {
      |                        ~~~~~~~~~~~^~~~
      |                        operator""_asv

Clang warns about this.  Apparently GCC 4.9 may warn about the
fixed syntax, though!
@lidavidm lidavidm marked this pull request as draft October 15, 2024 00:36
Copy link
Contributor

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm. Meson coverage error seems unrelated

@lidavidm lidavidm marked this pull request as ready for review October 15, 2024 01:02
@paleolimbot
Copy link
Member

Yes, I think this happened because it fails to compile on GCC 4.8 this way:

# export NANOARROW_ARCH=arm64      
# export NANOARROW_PLATFORM=centos7
# docker compose run --rm verify   
[ 14%] Building CXX object CMakeFiles/nanoarrow_testing.dir/src/nanoarrow/testing/testing.cc.o
In file included from /nanoarrow/src/nanoarrow/nanoarrow_testing.hpp:25:0,
                 from /nanoarrow/src/nanoarrow/testing/testing.cc:20:
/nanoarrow/src/nanoarrow/nanoarrow.hpp:95:24: error: missing space between ‘""’ and suffix identifier
 inline ArrowStringView operator""_asv(const char* data, std::size_t size_bytes) {
                        ^
/nanoarrow/src/nanoarrow/testing/testing.cc:2312:1: error: expected ‘}’ at end of input
 }  // namespace nanoarrow
 ^
/nanoarrow/src/nanoarrow/testing/testing.cc:2312:1: error: expected ‘}’ at end of input
cc1plus: warning: unrecognized command line option "-Wno-misleading-indentation" [enabled by default]
gmake[2]: *** [CMakeFiles/nanoarrow_testing.dir/src/nanoarrow/testing/testing.cc.o] Error 1
gmake[1]: *** [CMakeFiles/nanoarrow_testing.dir/all] Error 2
gmake: *** [all] Error 2
Failed to verify release candidate. See /tmp/nanoarrow-HEAD.qjyj8h for details.

Because we still check that the tests pass when compiled on gcc 4.8 and we use _asv in the tests, we probably need this to be something like:

#if !defined(__clang__) && (defined(__GNUC__) && __GNUC__ < 6)
inline ArrowStringView operator"" _asv(const char* data, std::size_t size_bytes) {
#else
inline ArrowStringView operator""_asv(const char* data, std::size_t size_bytes) {
#endif

...Or we could move that to the gtest_util header where it is less likely to cause problems for downstream projects. That would be a breaking change but I think that is OK.

@lidavidm
Copy link
Member Author

Ah, bleh. I'll try with the ifdef in a bit

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

I opened an issue for the Meson coverage build failure: #663

@paleolimbot paleolimbot merged commit 206e0e3 into apache:main Oct 15, 2024
34 of 35 checks passed
@lidavidm lidavidm deleted the minor-clang branch October 15, 2024 15:08
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.

3 participants