apache / arrow

Apache Arrow is the universal columnar format and multi-language toolbox for fast data interchange and in-memory analytics
https://arrow.apache.org/
Apache License 2.0
14.52k stars 3.53k forks source link

[C++] NDEBUG required for -fno-rtti #20296

Open asfimport opened 2 years ago

asfimport commented 2 years ago

Some of the code in checked_cast.h uses dynamic_cast unless NDEBUG is defined.  Defining NDEBUG affects other C++ functionality, for example it causes assert statements to be compiled out of the program.

It would be nice if Arrow provided some mechanism for compiling without RTTI (viz. not using dynamic_cast) without requiring NDEBUG to be defined and affecting assertions in other code.

 

Workaround 1: Compile without neither -fno-rtti nor NDEBUG in DEBUG mode, and with both -fno-rtti and NDEBUG in RELEASE mode.

Workaround 2: Include Arrow headers through a project header that defines NDEBUG, has the Arrow include statements, and then undefines NDEBUG.

Reporter: Jefferson Carpenter

Note: This issue was originally created as ARROW-16876. Please see the migration documentation for further details.

asfimport commented 2 years ago

Antoine Pitrou / @pitrou: 1) For the record, is there a particular reason for compiling without RTTI?

2) What does dynamic_cast do if RTTI is disabled?

asfimport commented 2 years ago

Jefferson Carpenter: 1) I'm compiling without rtti to save on binary size.  (Linux build is 15.2MB with RTTI / 15MB without RTTI on GCC 7.3.1, Mac build 11.1MB / 10.7MB on Apple clang version 13.0.0, so not a big difference).

2) dynamic_cast gives a compile error.

asfimport commented 2 years ago

Antoine Pitrou / @pitrou: Well, we could hide all uses of dynamic_cast behind a macro that would turn it into static_cast if RTTI isn't enabled (which apparently can be detected using the __cpp_rtti macro).

But that must be validated to work, potentially using a dedicated CI job...

cc @lidavidm, @bkietz for opinions.

asfimport commented 2 years ago

David Li / @lidavidm: If the savings is that minor, is it worth maintaining the extra configuration?

It looks like Parquet heavily uses dynamic_cast + there's one comment noting some compilers refuse to static_cast in a particular case..