Is it sometimes necessary to modify the upstream code to be able to package some C/C++ library?

Help
,
1

This is follow-up question after this thread.

I could successfully package minidocx for the Zig build system, write a C wrapper around its functions, and bind this wrapper to Zig. So I can now generate Word documents in my Zig project.

However, I could not just zig fetch that repo and use it as a dependendy directly. I needed to clone it, and modify some of the C++ code to fix the compilation error.

I looked into

\zig\0.14.1\files\lib\libcxx\include__chrono\

but I am not knowledgeable enough in Zig/C++/clang to understand if the changes I made were necessary, if there are some limitations due to Zig, clang, etc…, or if I could have just used some compilation flags to solve those errors instead.

Currently, I only use those flags:

        .flags = &.{ "-std=c++20", "-DMINIDOCX_NAMESPACE=md" },

and the errors were:

   └─ zig build-lib minidocx Debug native 5 errors
C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:119:13: error: enumeration value 'None' not handled in switch
    switch (openMode) {
            ^
C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:462:43: error: no member named 'clock_cast' in namespace 'std::chrono'
    fs::last_write_time(dst, std::chrono::clock_cast<std::chrono::file_clock>(mtime));
                             ~~~~~~~~~~~~~^
C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:462:77: error: expected '(' for function-style cast or type construction
    fs::last_write_time(dst, std::chrono::clock_cast<std::chrono::file_clock>(mtime));
                                                     ~~~~~~~~~~~~~~~~~~~~~~~^
C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:529:20: error: no member named 'clock_cast' in namespace 'std::chrono'
      std::chrono::clock_cast<std::chrono::system_clock>(fs::last_write_time(src)));
      ~~~~~~~~~~~~~^
C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:529:56: error: expected '(' for function-style cast or type construction
      std::chrono::clock_cast<std::chrono::system_clock>(fs::last_write_time(src)));
                              ~~~~~~~~~~~~~~~~~~~~~~~~~^

  1. 1st error enumeration value 'None' not handled in switch has an obvious fix, but is there a clang flag to prevent this error or maybe turn it into a warning instead?

  2. Concerning the std::chrono errors, is there a way in build.zig to use a different libc++ that has clock_cast as a member of std::chrono?

In the meantime I checked std::chrono and made this change:

-    fs::last_write_time(dst, std::chrono::clock_cast<std::chrono::file_clock>(mtime));
+    fs::last_write_time(dst, std::chrono::file_clock::from_sys(mtime));
  1. This did not work with the other clock_cast error though, while ::from_sys does not complain, ::to_sys throws an error. Why is there a difference there when clock_cast seems to work both ways even with lossy conversions?
-    addFileFromStream(name, fin,
-      std::chrono::clock_cast<std::chrono::system_clock>(fs::last_write_time(src)));
+    addFileFromStream(name, fin,
+      std::filesystem::file_time_type::clock::to_sys(fs::last_write_time(src)));

gave the following error:

C:\Users\user\dev\zig\test\minidocx\src/utils/zip.cpp:534:34: error: no viable conversion from 'time_point<[...], duration<__int128, rat
io<[...], 1000000000>>>' to 'const time_point<[...], duration<long long, ratio<[...], 1000000>>>'
    addFileFromStream(name, fin, std::filesystem::file_time_type::clock::to_sys(fs::last_write_time(src)));
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1. Going back to std::chrono, i tried std::chrono::duration_cast<>, but that was not recognized. Is there a reason why duration_cast is not recognized while the others 3 possibilities (round, floor, ceil) are?

In the end, my final changes were:

-    addFileFromStream(name, fin,
-      std::chrono::clock_cast<std::chrono::system_clock>(fs::last_write_time(src)));
+    const std::chrono::time_point tmp_sys_time = std::filesystem::file_time_type::clock::to_sys(fs::last_write_time(src));
+    using sys_clock_duration = std::chrono::system_clock::time_point::duration;
+    const std::chrono::system_clock::time_point sys_time = std::chrono::round<sys_clock_duration>(tmp_sys_time);
+    addFileFromStream(name, fin, sys_time);

I am sorry if those 4 questions are more about C++ than Zig, but I really would like to better understand the inner working of the zig build system when packaging C/C++ libraries, and to know if it is sometimes necessary to modify the original C/C++ code.

2

‘enumeration value ‘None’ not handled in switch’ looks like the code is compiled with -Werror (which turns warnings into errors), I would be surprised if the Zig build system enables that setting by default (if that is indeed the case, maybe specifically try building with -Wno-error but not sure if that is a valid command line option, because usually it’s used like -Wno-error=foo to define an exception when -Werror is enabled.

The other error looks like clock_cast hasn’t been implemented in Clang’s libc++.

Basically if some C++ code doesn’t compile in Zig, then that same code most likely also won’t build in a Clang toolchain.

PS: the CMakePresets.json file looks like the library has only been tested against MSVC and GCC, but not Clang:

3

Thanks for the confirmation. I did read somewhere that Zig turns warnings into errors, but adding -Werror displayed a lot more of those enumeration value 'None' not handled in switch.

I am not sure why it 1st only displayed this error in the file with the clock_cast error. However, after fixing the clock_cast errors, this enumeration error also disappeared.

I knew about clang/gcc but hadn’t realized that they could implement different parts of the standard. I definitely got some reading to do but I now understand why it is sometimes necessary to modify the upstream code to satisfy the clang toolchain :+1:

4

IME C++17 is the latest C++ standard that’s ‘safe to use’ across compiler toolchains. Anything above that I would expect holes in some less used parts of the stdlib.

PS: this is basically the ‘caniuse’ for C++ compilers: C++ compiler support - cppreference.com, and it indeed looks like Clang has some red and yellow entries in C++20.

2 Likes
5

Not sure if there is a better way to do it, nor if the repo owner will accept such a pull request but I ended up wrapping the modified part of the code in a #ifdef CLANG:

     fout.close();
+#ifdef CLANG
+    fs::last_write_time(dst, std::chrono::file_clock::from_sys(mtime));
+#else
     fs::last_write_time(dst, std::chrono::clock_cast<std::chrono::file_clock>(mtime));
+#endif
   }
[...]

+#ifdef CLANG
+    const std::chrono::time_point sys_time =
+        std::filesystem::file_time_type::clock::to_sys(fs::last_write_time(src));
+    using sys_clock_duration = std::chrono::system_clock::time_point::duration;
+    addFileFromStream(name, fin, 
+      std::chrono::round<sys_clock_duration>(sys_time));
+#else
     addFileFromStream(name, fin,
       std::chrono::clock_cast<std::chrono::system_clock>(fs::last_write_time(src)));
+#endif
   }

and adding the -DCLANG flag to addCSourceFiles()