mudge / re2

Ruby bindings to RE2, a "fast, safe, thread-friendly alternative to backtracking regular expression engines like those used in PCRE, Perl, and Python".
http://mudge.name/re2/
BSD 3-Clause "New" or "Revised" License
129 stars 13 forks source link

Fix "non-trivial designated initializers not supported" errors #117

Closed stanhu closed 9 months ago

stanhu commented 9 months ago

With g++ v7.3.1 (used with Amazon Linux 2), compiling the extension would fail with these errors:

compiling re2.cc
re2.cc:176:1: sorry, unimplemented: non-trivial designated initializers not
supported
 };
 ^
re2.cc:221:1: sorry, unimplemented: non-trivial designated initializers not
supported
 };
 ^
re2.cc:251:1: sorry, unimplemented: non-trivial designated initializers not
supported
 };
 ^
re2.cc:1610:1: sorry, unimplemented: non-trivial designated initializers not
supported
 };
 ^

This appears to be happening because the parent and data fields in rb_data_type_struct were omitted.

According to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55606, this issue was fixed in GCC 8.

mudge commented 9 months ago

Thanks for raising this, @stanhu. Am I right in thinking support for designated initialisers in C++ is a bit uncertain? Would we instead be better using the more "boring" form with only ordered expressions ala https://github.com/ruby/ruby/blob/60e19a0b5fc9c067ee88751192dc56da618f5060/weakmap.c#L151-L160?

For reference, here are the two versions of rb_data_type_t we need to support:

mudge commented 9 months ago

@stanhu I've pushed a commit to switch away from designated initializers: can you please test this and let me know if it works with g++ 7.3.1?

stanhu commented 9 months ago

Sounds good, thanks!

mudge commented 9 months ago

I've now published 2.4.3 to fix this. Please let me know if there are any other issues!

mudge commented 9 months ago

@byroot this issue and the two potential fixes (see the two commits) may be of interest to you if you’re trying to convert other C++ extensions from Data to TypedData.

byroot commented 9 months ago

Thanks for the ping, this is good to know. But hopefully there isn't too many of those around 😄