Skip to content

fix: remove deprecated operator"" syntax usage#661

Merged
paleolimbot merged 2 commits into
apache:mainfrom
lidavidm:minor-clang
Oct 15, 2024
Merged

fix: remove deprecated operator"" syntax usage#661
paleolimbot merged 2 commits into
apache:mainfrom
lidavidm:minor-clang

Conversation

@lidavidm

@lidavidm lidavidm commented Oct 15, 2024

Copy link
Copy Markdown
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

@lidavidm
fix: remove deprecated operator"" syntax usage
e95fc02 
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
WillAyd
WillAyd approved these changes Oct 15, 2024
View reviewed changes

@WillAyd WillAyd left a comment

Copy link
Copy Markdown
Contributor

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

paleolimbot commented Oct 15, 2024

Copy link
Copy Markdown
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

lidavidm commented Oct 15, 2024

Copy link
Copy Markdown
Member Author

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

paleolimbot
paleolimbot approved these changes Oct 15, 2024
View reviewed changes

@paleolimbot paleolimbot 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.

Thank you!

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

@paleolimbot paleolimbot merged commit 206e0e3 into apache:main Oct 15, 2024
@lidavidm lidavidm deleted the minor-clang branch October 15, 2024 15:08
@jyknight jyknight mentioned this pull request Oct 15, 2024
Merged
@r0qs r0qs mentioned this pull request Jun 1, 2025
Merged
@paleolimbot paleolimbot added this to the nanoarrow 0.7.0 milestone Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WillAyd WillAyd WillAyd approved these changes

@paleolimbot paleolimbot paleolimbot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

nanoarrow 0.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@lidavidm @paleolimbot @WillAyd