Standalone 3/N: Define the Zarr logger

[aliddell]

Depends on #294.

[shlomnissan]

Overall, this PR looks good, but there are two issues I consider blockers:

• Having underlying types that belong to one target defined in another creates potentially unnecessary coupling. While not critical, it's worth addressing.
• More importantly, using std::localtime in a multi-threaded environment is likely a bug. We should double-check this before merging.

[shlomnissan]

A couple of nits and FYIs:

• I believe this target name can be generalized to acquire-logger. It doesn't need to be Zarr-specific.
• Directly using the target name instead of a variable improves readability, maintainability, and the overall robustness of your CMake configuration. The indirection of a variable is unnecessary in most use cases and can introduce subtle bugs or confusion if you have multiple targets. Most of the time people use it to reduce redundancy, but it's typically unnecessary.

[shlomnissan]

The fact that these types are not defined within the logger target compromises encapsulation. If the logger is defined as a standalone target, it should be self-contained and reusable. Could we create an additional header file, perhaps named logger_types.h, with a C interface to address this issue?

[shlomnissan]

If this is only used once, we could inline this variable.

[shlomnissan]

This relates to the filtering issue discussed in the previous PR. My preferred API would allow users to enable or disable logs while handling filtering themselves. I find the current pattern suboptimal because it assumes users know our predefined log levels and lacks flexibility. For instance, a user might want to see info logs but not debug logs.

[shlomnissan]

I mentioned this in the comprehensive PR review, but a modern approach to handling an arbitrary number of arguments is using variadic templates.

Aspect	va_list (C)	Variadic Templates (C++)
Type Safety	None, relies on the programmer to manage types	Full type safety, checked by the compiler
Runtime vs Compile-time	Arguments processed at runtime	Arguments processed at compile time
Ease of Use	Requires manual handling with macros	Recursion or fold expressions make usage simpler
Flexibility	Limited to basic types and needs type casting	Supports any type (built-in and user-defined)
Error Handling	Type errors lead to runtime crashes or UB	Errors caught at compile-time
Performance	Slower (runtime argument parsing)	Faster (resolved at compile-time)
Common Use Cases	C-style printf, legacy C functions	Generic functions or classes in modern C++

[shlomnissan]

Suggested change

	std::string prefix;
	std::ostream* stream = &std::cout;
	auto prefix = std::string {};
	auto& stream = &std::cout;

[shlomnissan]

Why would a warning message go to std::cerr?

[shlomnissan]

Add default case for completeness. Are we using a code linter like clangd/clang-tidy or sonar? This issue should be flagged by such tools.

[shlomnissan]

I think I mentioned this in the large PR review. I would extract this code and std::put_time to a separate function. This snippet is copied from a logger that I wrote a while back so it may need to be adjusted a bit if you decide to use it.

auto GetTimestamp() -> std::string {
    using std::chrono::system_clock;
    auto now = system_clock::now();
    auto in_time_t = system_clock::to_time_t(now);

    auto time_info = std::tm{};
    #ifdef _WIN32
        localtime_s(&time_info, &in_time_t);
    #else
        localtime_r(&in_time_t, &time_info);
    #endif

    return fmt::format("{:04}-{:02}-{:02} {:02}:{:02}:{:02}",
                       time_info.tm_year + 1900,
                       time_info.tm_mon + 1,
                       time_info.tm_mday,
                       time_info.tm_hour,
                       time_info.tm_min,
                       time_info.tm_sec);
}

[shlomnissan]

I think that std::localtime is not thread-safe. See my previous code snippet that uses localtime_s or localtime_r.

Function	Thread Safety	Availability	Usage
std::localtime	Not thread-safe	C++ standard (all platforms)	Uses static struct tm, may cause data races in multi-threaded environments.
std::localtime_s	Thread-safe	Windows, C11	Caller provides struct tm. Part of Microsoft's C++ library, available on Windows.
std::localtime_r	Thread-safe	POSIX (Linux/macOS)	Caller provides struct tm. Commonly used in Unix-like systems.

[shlomnissan]

LGTM!