Add -D_FILE_OFFSET_BITS=64 #799
Conversation
WalkthroughThe changes add the compiler flag Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Just unconditionally enable it, there's no reason to use 32-bit file offsets even on 32-bit systems. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
m4/acx_64bit_file_offset.m4 (1)
4-4: Use consistent naming convention for the configure option.The help string uses
--enable_64bit_file_offsetwith underscores, but Autoconf configure options typically use hyphens. Consider using--enable-64bit-file-offsetfor consistency with standard conventions.- [AS_HELP_STRING([--enable_64bit_file_offset],[enable 64 bit file offset mode @<:@enabled@:>@])], + [AS_HELP_STRING([--enable-64bit-file-offset],[enable 64 bit file offset mode @<:@enabled@:>@])],Note: If you make this change, also update the variable name from
enable_64bit_file_offsettoenable_64bit_file_offset(Autoconf automatically converts hyphens to underscores in variable names).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
configure.ac(1 hunks)m4/acx_64bit_file_offset.m4(1 hunks)
🔇 Additional comments (2)
m4/acx_64bit_file_offset.m4 (1)
9-10: LGTM: Correct implementation of 64-bit file offset support.The macro correctly appends
-D_FILE_OFFSET_BITS=64to both C and C++ compiler flags, which is the standard approach for enabling large file support on systems that require it.configure.ac (1)
53-53: LGTM: Proper integration of the new macro.The macro is correctly invoked in the appropriate section alongside other compiler flag configuration macros. The placement after
ACX_64BITis logical and maintains good organization.Note: Ensure that if the macro parameter name is corrected in
m4/acx_64bit_file_offset.m4(changing "strict" to "64bit_file_offset"), this invocation will continue to work correctly.
LawrenceHunter
force-pushed
the
lawrence/64bit_file_offset
branch
from
b24c880 to
92ba5c3
Compare
Okay I'll set it enabled by default |
LawrenceHunter
force-pushed
the
lawrence/64bit_file_offset
branch
from
92ba5c3 to
29d0650
Compare
No, don't even make it configurable. Hardcode it in CFLAGS. |
I can't see a place where this would be suitable to add as they're all under the *.m4 files the current implementation makes it the same as strict |
|
I'm not fluent in autotools but I guess it would go into |
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
Nice.
Either remove the AC_ARG_ENABLE part, or just add the flags in configure.ac directly, like:
# Compiler flags
CFLAGS="${CFLAGS} -D_FILE_OFFSET_BITS=64"
CXXFLAGS="${CXXFLAGS} -D_FILE_OFFSET_BITS=64"
ACX_PEDANTIC
ACX_STRICT
ACX_64BIT
AX_CXX_COMPILE_STDCXX_11([noext],[optional])
A similar change for CMake should fit in cmake/modules/CompilerOptions.cmake.
LawrenceHunter
force-pushed
the
lawrence/64bit_file_offset
branch
from
29d0650 to
c8de1f2
Compare
|
Thanks for the advice both, it is now included in the flags. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/modules/CompilerOptions.cmake (1)
57-58: LGTM! Consider using modern CMake syntax for better maintainability.The change correctly enables 64-bit file offsets to resolve the 32-bit readdir issue. However, consider using the more idiomatic CMake approach:
-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -D_FILE_OFFSET_BITS=64") -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_FILE_OFFSET_BITS=64") +add_compile_definitions(_FILE_OFFSET_BITS=64)This approach is cleaner, more maintainable, and automatically handles the definition for all languages without string manipulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmake/modules/CompilerOptions.cmake(1 hunks)configure.ac(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- configure.ac
LawrenceHunter
changed the title
4 participants
Fixes an error when running a 32 bit version of SoftHSM where readdir fails with errno 75.
Summary by CodeRabbit