chaokunyang commented on code in PR #3789:
URL: https://github.com/apache/fory/pull/3789#discussion_r3474403183
##########
cpp/fory/serialization/temporal_serializers.h:
##########
@@ -315,5 +358,142 @@ template <> struct Serializer<Date> {
}
};
+// ============================================================================
+// Chrono serializers
+//
+// These allow users to explicitly request chrono types as the deserialization
+// target (e.g., fory.deserialize<std::chrono::nanoseconds>(...)). They share
+// the same wire encoding as the Fory-owned carrier serializers above and
+// delegate to them via conversion
+// ============================================================================
+
+/// Serializer for std::chrono::nanoseconds
+/// Per xlang spec: serialized as signed varint64 seconds + signed int32
+/// nanoseconds
+template <> struct Serializer<std::chrono::nanoseconds> {
Review Comment:
This makes chrono temporal types valid Any registrations for the same
internal TypeId as the Fory carriers. Since `register_any_type` installs
`any_read_fn` on the shared `TypeInfo` for `DURATION`/`TIMESTAMP`, the dynamic
deserialization carrier becomes registration-order dependent. We should keep
Any/polymorphic reads canonical: `DURATION`/`TIMESTAMP` should deserialize to
the Fory carrier types, while chrono remains an explicit static target
adaptation. Please add coverage for `std::any` duration/timestamp and prevent
chrono registration from overriding the canonical carrier.
##########
cpp/fory/serialization/temporal_serializers.h:
##########
@@ -21,21 +21,63 @@
#include "fory/serialization/serializer.h"
#include <chrono>
+#include <functional>
#include <limits>
namespace fory {
namespace serialization {
// ============================================================================
-// Temporal Type Aliases
+// Fory-owned temporal carrier types
// ============================================================================
/// Duration: absolute length of time as nanoseconds
-using Duration = std::chrono::nanoseconds;
+class Duration {
Review Comment:
Now that `Duration`/`Timestamp` are Fory-owned carrier types, they should
not be defined in the serializer header/namespace. Please move the value types
to `cpp/fory/type`, expose the canonical API as `fory::Duration` /
`fory::Timestamp` / `fory::Date`, and leave aliases in `fory::serialization` if
needed for existing call sites. `temporal_serializers.h` should only own the
`Serializer` specializations and chrono adapters.
##########
docs/guide/cpp/supported-types.md:
##########
@@ -189,29 +189,44 @@ OptionalInt value = 42;
## Temporal Types
+`Duration`, `Timestamp`, and `Date` are Fory-owned carrier types defined in
Review Comment:
Please update the canonical type-mapping docs as part of this public carrier
change. The C++ guide now describes Fory-owned carriers, but
`docs/specification/xlang_type_mapping.md` still maps C++ timestamp to
`std::chrono::nanoseconds`. The docs should consistently say that FDL/codegen
and dynamic/Any use the Fory carrier by default, while chrono is an explicit
C++ adaptation target.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
