-
Notifications
You must be signed in to change notification settings - Fork 210
Added support for (CMake) Unity Builds. #94
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
Conversation
You have explained what you did, but not the why. These are internal defines, should not affect other libs / programs. |
In general, I find it convenient to have the option there, to use unity builds (often they are faster), so I thought it might be a good addition to PCRE2. |
while I doubt a unity build of PCRE2 would be faster than a regular build with Ninja or even make, the change itself seems sensible and unlikely to do any harm (except maybe by the odd renaming of the pcre2_convert enum and the ongoing maintenance). @pkeir: could you elaborate better in the rationale to understand if breaking that is worth the tradeoff?, for example that enum that was renamed could had been deleted as well, and the way the codebase is structured, there are a lot more unusual includes (ex: pcre2_jit_compile.c includes another c file inside sljit that includes several other files) so the scope of macros is not as straight forward as you would expect in applications that only include headers with well defined APIs. |
@carenas Apologies for the delay, I missed the notification. I would firstly suggest that a CMake unity build can also be a default option for some developers; regardless of whether the build speed is improved. For those users, it will be a reassurance, as (rightly or wrongly) the option of a unity build can be an indicator of good code hygiene. As for the details of the commit: indeed, the enum could have been removed rather than renamed. I chose a conservative route to allow a simple translation for imagined authors of pending PCRE2 modules. I would otherwise support its removal. In fact, reviewing the changes again reminds me that a grep for RANGE_STARTED produces results, which for a newcomer, could unexpectedly have come from either of two different enums. I note the include of sljitLir.c in pcre2_jit_compile.c you mention. I can only say that I also built with JIT enabled, and all of the test sets passed as before; plus extra passes for the enabled JIT feature tests (including pcre2_jit_test). One last thought is that unity builds can also simplify some integration tasks; for example when compiling as a C++ constexpr module, or a GPGPU program. |
OK, as @carenas says, this change doesn't look harmful. I will shortly merge it, but then I am going to remove the unused enum, which must be an oversight, and I'm also going to add comments to remind me (or anyone else) why there are #undefs at the end of modules. I will experiment with a unity build under Linux, and if that's easy, add it to my list of pre-release tests, because I can see how easy it would be to break this by accident. |
When support for Unity/Jumbo builds was added[1], the fact that cmake will need to be able to not mix files with different PCRE2_UCHAR sizes was missed, resulting in a possibly broken build by redefining LINK_SIZE as shown by warnings during compilation. Since 4678857 (add a C23 inspired checked integer multiplication helper (PCRE2Project#198), 2023-02-03), the build will fail if the linker wouldn't be able to merge the multiple implementations of pcre2_ckd_smul from each participating library. To avoid both problems, disable UNITY_BUILD for the non 8-bit libraries. [1] PCRE2Project#94
When support for Unity/Jumbo builds was added[1], the fact that cmake will need to be able to not mix files with different PCRE2_UCHAR sizes was missed, resulting in a possibly broken build by redefining LINK_SIZE as shown by warnings during compilation. Since 4678857 (add a C23 inspired checked integer multiplication helper (#198), 2023-02-03), the build will fail if the linker wouldn't be able to merge the multiple implementations of pcre2_ckd_smul from each participating library. To avoid both problems, disable UNITY_BUILD for the non 8-bit libraries. [1] #94
The modifications allow a unity (jumbo) build by renaming an enum member (
RANGE_STARTED
); adding#undef
forNLBLOCK
,PSSTART
, andPSEND
at the end of the (three) units where they are defined; and renaming two of the threedo_callout
definitions.CMake can then be used to create a unity build as shown:
cmake -DCMAKE_UNITY_BUILD=ON ..