[libc] Implement strftime #111305

tszhin-swe · 2024-10-06T17:19:35Z

[libc] Implement strftime. #106630

graphite-app · 2024-10-06T17:20:20Z

libc/src/stdio/strftime_core/converter.cpp

+                                             const FormatSection &to_conv) {
+  return details::write_num<1>(to_conv.time->tm_wday == 0 ? 7 : to_conv.time->tm_wday, writer);


The conversion for %w should return 0 for Sunday, not 7. The condition can be simplified to:

return details::write_num<1>(to_conv.time->tm_wday, writer);

This change aligns with the POSIX specification for strftime, where Sunday is represented by 0.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

github-actions · 2024-10-06T17:23:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Summary: This patch does not actually implement the function, but provides the interface. This partially resovles llvm#106630 as it will allow us to build libc++ with locale support (so long as you don't use it).

graphite-app · 2024-10-13T15:40:53Z

libc/src/time/strftime.cpp

+  printf_core::WriteBuffer wb(buffer, (buffsz > 0 ? buffsz - 1 : 0));
+  printf_core::Writer writer(&wb);
+  strftime_core::strftime_main(&writer, format, timeptr);
+  return -1;


The current implementation always returns -1, which is incorrect. The function should return the number of characters written, or 0 if the result doesn't fit in the buffer. Consider replacing return -1; with return writer.get_chars_written();. Additionally, you may want to add logic to handle buffer overflow and return 0 in such cases.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

llvmbot · 2024-10-23T14:32:00Z

@llvm/pr-subscribers-libc

Author: Tsz Chan (tszhin-swe)

Changes

Patch is 61.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/111305.diff

24 Files Affected:

(modified) libc/config/gpu/entrypoints.txt (+2)
(modified) libc/newhdrgen/yaml/time.yaml (+20)
(modified) libc/spec/stdc.td (+21)
(modified) libc/src/time/CMakeLists.txt (+30-4)
(added) libc/src/time/strftime.cpp (+31)
(added) libc/src/time/strftime.h (+23)
(added) libc/src/time/strftime_core/CMakeLists.txt (+57)
(added) libc/src/time/strftime_core/composite_converter.h (+124)
(added) libc/src/time/strftime_core/converter.cpp (+73)
(added) libc/src/time/strftime_core/converter.h (+28)
(added) libc/src/time/strftime_core/core_structs.h (+39)
(added) libc/src/time/strftime_core/num_converter.h (+208)
(added) libc/src/time/strftime_core/parser.h (+106)
(added) libc/src/time/strftime_core/str_converter.h (+55)
(added) libc/src/time/strftime_core/strftime_main.cpp (+43)
(added) libc/src/time/strftime_core/strftime_main.h (+32)
(added) libc/src/time/strftime_core/time_internal_def.h (+77)
(added) libc/src/time/strftime_l.cpp (+32)
(added) libc/src/time/strftime_l.h (+24)
(modified) libc/test/src/time/CMakeLists.txt (+12)
(added) libc/test/src/time/strftime_core/CMakeLists.txt (+23)
(added) libc/test/src/time/strftime_core/converter_test.cpp (+481)
(added) libc/test/src/time/strftime_core/parser_test.cpp (+102)
(added) libc/test/src/time/strftime_test.cpp (+113)

diff --git a/libc/config/gpu/entrypoints.txt b/libc/config/gpu/entrypoints.txt
index b4cfe47f4505fd..af05dc57023f08 100644
--- a/libc/config/gpu/entrypoints.txt
+++ b/libc/config/gpu/entrypoints.txt
@@ -256,6 +256,8 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.time.clock
     libc.src.time.clock_gettime
     libc.src.time.nanosleep
+    libc.src.time.strftime
+    libc.src.time.strftime_l
 
     # wchar.h entrypoints
     libc.src.wchar.wctob
diff --git a/libc/newhdrgen/yaml/time.yaml b/libc/newhdrgen/yaml/time.yaml
index 69b40bef3160dd..593b52b2d5a704 100644
--- a/libc/newhdrgen/yaml/time.yaml
+++ b/libc/newhdrgen/yaml/time.yaml
@@ -8,6 +8,7 @@ types:
   - type_name: time_t
   - type_name: clock_t
   - type_name: size_t
+  - type_name: locale_t
 enums: []
 objects: []
 functions:
@@ -96,3 +97,22 @@ functions:
     return_type: time_t
     arguments:
       - type: time_t *
+  - name: strftime
+    standard:
+      - stdc
+    return_type: size_t
+    arguments:
+      - type: char *__restrict
+      - type: size_t
+      - type: const char *__restrict
+      - type: const struct tm *__restrict
+  - name: strftime_l
+    standard:
+      - stdc
+    return_type: size_t
+    arguments:
+      - type: char *__restrict
+      - type: size_t
+      - type: const char *__restrict
+      - type: const struct tm *__restrict
+      - type: locale_t
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index 7caf543748151a..f673e0e3bc246a 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -1586,6 +1586,7 @@ def StdC : StandardSpec<"stdc"> {
          StructTimeSpec,
          TimeTType,
          SizeTType,
+         LocaleT,
       ],
       [], // Enumerations
       [
@@ -1651,6 +1652,26 @@ def StdC : StandardSpec<"stdc"> {
               RetValSpec<TimeTType>,
               [ArgSpec<TimeTTypePtr>]
           >,
+          FunctionSpec<
+              "strftime",
+              RetValSpec<SizeTType>,
+              [
+                  ArgSpec<CharRestrictedPtr>,
+                  ArgSpec<SizeTType>,
+                  ArgSpec<ConstCharRestrictedPtr>,
+                  ArgSpec<StructTmPtr>
+              ]
+          FunctionSpec<
+              "strftime_l",
+              RetValSpec<SizeTType>,
+              [
+                  ArgSpec<CharRestrictedPtr>,
+                  ArgSpec<SizeTType>,
+                  ArgSpec<ConstCharRestrictedPtr>,
+                  ArgSpec<StructTmPtr>,
+                  ArgSpec<LocaleT>
+              ]
+          >,
       ]
   >;
 
diff --git a/libc/src/time/CMakeLists.txt b/libc/src/time/CMakeLists.txt
index b3318e7ca87fa5..6e6c21bc657386 100644
--- a/libc/src/time/CMakeLists.txt
+++ b/libc/src/time/CMakeLists.txt
@@ -1,6 +1,6 @@
-if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
-  add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
-endif()
+# if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+#   add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+# endif()
 
 add_object_library(
   time_utils
@@ -99,7 +99,6 @@ add_entrypoint_object(
   HDRS
     mktime.h
   DEPENDS
-    .time_utils
     libc.include.time
     libc.src.errno.errno
 )
@@ -138,3 +137,30 @@ add_entrypoint_object(
   DEPENDS
     .${LIBC_TARGET_OS}.gettimeofday
 )
+
+add_subdirectory(strftime_core)
+
+add_entrypoint_object(
+  strftime
+  SRCS
+    strftime.cpp
+  HDRS
+    strftime.h
+  DEPENDS
+    libc.include.time
+    libc.src.time.strftime_core.strftime_main
+    libc.src.stdio.printf_core.writer
+)
+
+add_entrypoint_object(
+  strftime_l
+  SRCS
+    strftime_l.cpp
+  HDRS
+    strftime_l.h
+  DEPENDS
+    libc.include.time
+    libc.include.locale
+    libc.src.time.strftime_core.strftime_main
+    libc.src.stdio.printf_core.writer
+)
\ No newline at end of file
diff --git a/libc/src/time/strftime.cpp b/libc/src/time/strftime.cpp
new file mode 100644
index 00000000000000..5035001e6fb6b7
--- /dev/null
+++ b/libc/src/time/strftime.cpp
@@ -0,0 +1,31 @@
+//===-- Implementation of strftime function -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/time/strftime.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+#include "src/errno/libc_errno.h"
+#include "src/time/time_utils.h"
+
+#include "src/stdio/printf_core/writer.h"
+#include "src/time/strftime_core/strftime_main.h"
+namespace LIBC_NAMESPACE_DECL {
+
+size_t strftime(char *__restrict buffer, size_t buffsz,
+                const char *__restrict format, const struct tm *timeptr) {
+
+  printf_core::WriteBuffer wb(buffer, (buffsz > 0 ? buffsz - 1 : 0),
+                              strftime_core::overflow_write_mock, nullptr);
+  printf_core::Writer writer(&wb);
+  int ret = strftime_core::strftime_main(&writer, format, timeptr);
+  if (buffsz > 0) // if the buffsz is 0 the buffer may be a null pointer.
+    wb.buff[wb.buff_cur] = '\0';
+  return ret > 0 ? ret : 0;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/time/strftime.h b/libc/src/time/strftime.h
new file mode 100644
index 00000000000000..aa30336d8957d3
--- /dev/null
+++ b/libc/src/time/strftime.h
@@ -0,0 +1,23 @@
+//===-- Implementation header of strftime -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_TIME_STRFTIME_H
+#define LLVM_LIBC_SRC_TIME_STRFTIME_H
+
+#include "src/__support/macros/config.h"
+#include <stddef.h>
+#include <time.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+size_t strftime(char *__restrict, size_t max, const char *__restrict format,
+                const struct tm *timeptr);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_TIME_STRFTIME_H
diff --git a/libc/src/time/strftime_core/CMakeLists.txt b/libc/src/time/strftime_core/CMakeLists.txt
new file mode 100644
index 00000000000000..79e2eafc6a98de
--- /dev/null
+++ b/libc/src/time/strftime_core/CMakeLists.txt
@@ -0,0 +1,57 @@
+add_header_library(
+  core_structs
+  HDRS
+    core_structs.h
+  DEPENDS
+    libc.src.__support.CPP.string_view
+    libc.include.time
+)
+
+add_header_library(
+  parser
+  HDRS
+    parser.h
+  DEPENDS
+    .core_structs
+    libc.src.string.string_utils
+    libc.include.time
+
+)
+
+add_object_library(
+  converter
+  SRCS
+    converter.cpp
+  HDRS
+    converter.h
+    num_converter.h
+    str_converter.h
+    composite_converter.h
+  DEPENDS
+    .core_structs
+    libc.src.__support.arg_list
+    libc.src.stdio.printf_core.writer
+    libc.src.stdio.printf_core.printf_main
+    libc.src.__support.big_int
+    libc.src.__support.CPP.string_view
+    libc.src.__support.float_to_string
+    libc.src.__support.integer_to_string
+    libc.src.__support.uint128
+    libc.src.__support.StringUtil.error_to_string
+    libc.include.time
+
+)
+
+add_object_library(
+  strftime_main
+  SRCS
+    strftime_main.cpp
+  HDRS
+    strftime_main.h
+  DEPENDS
+    .core_structs
+    .parser
+    .converter
+    libc.src.stdio.printf_core.writer
+    libc.include.time
+)
\ No newline at end of file
diff --git a/libc/src/time/strftime_core/composite_converter.h b/libc/src/time/strftime_core/composite_converter.h
new file mode 100644
index 00000000000000..cf317988de0757
--- /dev/null
+++ b/libc/src/time/strftime_core/composite_converter.h
@@ -0,0 +1,124 @@
+//===-- Format specifier converter for printf -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See htto_conv.times://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_COMPOSITE_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_COMPOSITE_CONVERTER_H
+
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/arg_list.h"
+#include "src/__support/integer_to_string.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/printf_core/printf_main.h"
+#include "src/stdio/printf_core/writer.h"
+#include "src/time/strftime_core/core_structs.h"
+#include "src/time/strftime_core/time_internal_def.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace strftime_core {
+
+namespace details {
+int snprintf_impl(char *__restrict buffer, size_t buffsz,
+                  const char *__restrict format, ...) {
+  va_list vlist;
+  va_start(vlist, format);
+  internal::ArgList args(vlist);
+  va_end(vlist);
+  printf_core::WriteBuffer wb(buffer, (buffsz > 0 ? buffsz - 1 : 0));
+  printf_core::Writer writer(&wb);
+
+  int ret_val = printf_core::printf_main(&writer, format, args);
+  if (buffsz > 0)
+    wb.buff[wb.buff_cur] = '\0';
+  return ret_val;
+}
+} // namespace details
+
+int write_composite(printf_core::Writer *writer, const FormatSection &to_conv) {
+  char buffer[100];
+  auto &time = *to_conv.time;
+
+  switch (to_conv.conv_name) {
+  // Full date and time representation (e.g., equivalent to %a %b %e %T %Y)
+  case 'c': {
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%s %s %02d %02d:%02d:%02d %d",
+        safe_abbreviated_day_name(time.tm_wday),
+        safe_abbreviated_month_name(time.tm_mon), time.tm_mday, time.tm_hour,
+        time.tm_min, time.tm_sec, time.tm_year + 1900));
+    break;
+  }
+
+  // Zero-padded day of the month (equivalent to %m/%d/%y)
+  case 'D': {
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%02d/%02d/%02d", time.tm_mon + 1, time.tm_mday,
+        (time.tm_year + 1900) % 100));
+    break;
+  }
+
+  // ISO 8601 date representation in YYYY-MM-DD (equivalent to %Y-%m-%d)
+  case 'F': {
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%04d-%02d-%02d", time.tm_year + 1900,
+        time.tm_mon + 1, time.tm_mday));
+    break;
+  }
+
+  // 12-hour clock time with seconds and AM/PM (equivalent to %I:%M:%S %p)
+  case 'r': {
+    int hour12 = time.tm_hour % 12;
+    if (hour12 == 0)
+      hour12 = 12;
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%02d:%02d:%02d %s", hour12, time.tm_min,
+        time.tm_sec,
+        to_conv.time->tm_hour >= 12 ? default_PM_str : default_AM_str));
+    break;
+  }
+
+  // 24-hour time without seconds (equivalent to %H:%M)
+  case 'R': {
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%02d:%02d", time.tm_hour, time.tm_min));
+    break;
+  }
+
+  // Time with seconds (equivalent to %H:%M:%S)
+  case 'T': {
+    RET_IF_RESULT_NEGATIVE(
+        details::snprintf_impl(buffer, sizeof(buffer), "%02d:%02d:%02d",
+                               time.tm_hour, time.tm_min, time.tm_sec));
+    break;
+  }
+
+  // Locale's date representation (often equivalent to %m/%d/%y)
+  case 'x': {
+    RET_IF_RESULT_NEGATIVE(details::snprintf_impl(
+        buffer, sizeof(buffer), "%02d/%02d/%02d", time.tm_mon + 1, time.tm_mday,
+        (time.tm_year + 1900) % 100));
+    break;
+  }
+
+  // Locale's time representation (equivalent to %H:%M:%S)
+  case 'X': {
+    RET_IF_RESULT_NEGATIVE(
+        details::snprintf_impl(buffer, sizeof(buffer), "%02d:%02d:%02d",
+                               time.tm_hour, time.tm_min, time.tm_sec));
+    break;
+  }
+
+  default:
+    return writer->write(to_conv.raw_string);
+  }
+  return writer->write(buffer);
+}
+
+} // namespace strftime_core
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif
diff --git a/libc/src/time/strftime_core/converter.cpp b/libc/src/time/strftime_core/converter.cpp
new file mode 100644
index 00000000000000..a9bbf6ceca63cb
--- /dev/null
+++ b/libc/src/time/strftime_core/converter.cpp
@@ -0,0 +1,73 @@
+//===-- Format specifier converter for printf -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See htto_conv.times://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CONVERTER_H
+
+#include "composite_converter.h"
+#include "num_converter.h"
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/integer_to_string.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/printf_core/writer.h"
+#include "src/time/strftime_core/core_structs.h"
+#include "src/time/strftime_core/time_internal_def.h"
+#include "str_converter.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace strftime_core {
+
+int convert(printf_core::Writer *writer, const FormatSection &to_conv) {
+  if (!to_conv.has_conv)
+    return writer->write(to_conv.raw_string);
+  switch (to_conv.conv_name) {
+  case '%':
+    return writer->write("%");
+  case 'C': // Century (C)
+  case 'Y': // Full year (Y)
+  case 'y': // Two-digit year (y)
+  case 'j': // Day of the year (j)
+  case 'm': // Month (m)
+  case 'd': // Day of the month (d)
+  case 'e': // Day of the month (e)
+  case 'H': // 24-hour format (H)
+  case 'I': // 12-hour format (I)
+  case 'M': // Minute (M)
+  case 'S': // Second (S)
+  case 'U': // Week number starting on Sunday (U)
+  case 'W': // Week number starting on Monday (W)
+  case 'V': // ISO week number (V)
+  case 'G': // ISO year (G)
+  case 'w': // Decimal weekday (w)
+  case 'u': // ISO weekday (u)
+    return write_num(writer, to_conv);
+  case 'a': // Abbreviated weekday name (a)
+  case 'A': // Full weekday name (A)
+  case 'b': // Abbreviated month name (b)
+  case 'B': // Full month name (B)
+  case 'p': // AM/PM designation (p)
+  case 'z': // Timezone offset (z)
+  case 'Z': // Timezone name (Z)
+    return write_str(writer, to_conv);
+  case 'c':
+  case 'F':
+  case 'r':
+  case 'R':
+  case 'T':
+  case 'x':
+  case 'X':
+    return write_composite(writer, to_conv);
+  default:
+    return writer->write(to_conv.raw_string);
+  }
+  return 0;
+}
+
+} // namespace strftime_core
+} // namespace LIBC_NAMESPACE_DECL
+#endif
diff --git a/libc/src/time/strftime_core/converter.h b/libc/src/time/strftime_core/converter.h
new file mode 100644
index 00000000000000..8809a8356e5c59
--- /dev/null
+++ b/libc/src/time/strftime_core/converter.h
@@ -0,0 +1,28 @@
+//===-- Format specifier converter for printf -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CONVERTER_H
+
+#include "src/stdio/printf_core/writer.h"
+#include "src/time/strftime_core/core_structs.h"
+
+#include <stddef.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace strftime_core {
+
+// convert will call a conversion function to convert the FormatSection into
+// its string representation, and then that will write the result to the
+// writer.
+int convert(printf_core::Writer *writer, const FormatSection &to_conv);
+
+} // namespace strftime_core
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CONVERTER_H
diff --git a/libc/src/time/strftime_core/core_structs.h b/libc/src/time/strftime_core/core_structs.h
new file mode 100644
index 00000000000000..5c01638f132da4
--- /dev/null
+++ b/libc/src/time/strftime_core/core_structs.h
@@ -0,0 +1,39 @@
+//===-- Core Structures for printf ------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CORE_STRUCTS_H
+#define LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CORE_STRUCTS_H
+
+#include "src/__support/CPP/string_view.h"
+#include <time.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace strftime_core {
+
+struct FormatSection {
+  bool has_conv{false};
+  bool isE{false};
+  bool isO{false};
+  cpp::string_view raw_string{};
+  char conv_name;
+  const struct tm *time;
+  int min_width{0};
+  char padding;
+};
+
+#define RET_IF_RESULT_NEGATIVE(func)                                           \
+  {                                                                            \
+    int result = (func);                                                       \
+    if (result < 0)                                                            \
+      return result;                                                           \
+  }
+
+} // namespace strftime_core
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_CORE_STRUCTS_H
diff --git a/libc/src/time/strftime_core/num_converter.h b/libc/src/time/strftime_core/num_converter.h
new file mode 100644
index 00000000000000..baad78a281b657
--- /dev/null
+++ b/libc/src/time/strftime_core/num_converter.h
@@ -0,0 +1,208 @@
+//===-- Format specifier converter for printf -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See htto_conv.times://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_NUM_CONVERTER_H
+#define LLVM_LIBC_SRC_STDIO_STRFTIME_CORE_NUM_CONVERTER_H
+
+#include "src/__support/CPP/string_view.h"
+#include "src/__support/integer_to_string.h"
+#include "src/__support/macros/config.h"
+#include "src/stdio/printf_core/writer.h"
+#include "src/time/strftime_core/core_structs.h"
+#include "src/time/strftime_core/time_internal_def.h"
+
+namespace LIBC_NAMESPACE_DECL {
+namespace strftime_core {
+
+namespace details {
+
+LIBC_INLINE cpp::optional<cpp::string_view>
+num_to_strview(uintmax_t num, cpp::span<char> bufref) {
+  return IntegerToString<uintmax_t>::format_to(bufref, num);
+}
+
+template <typename T> int count_digits(T num) {
+  if (num == 0)
+    return 1;
+  int digits = 0;
+  while (num > 0) {
+    num /= 10;
+    digits++;
+  }
+  return digits;
+}
+
+LIBC_INLINE int write_num_with_padding(int width, char padding, uintmax_t num,
+                                       printf_core::Writer *writer) {
+  cpp::array<char, IntegerToString<uintmax_t>::buffer_size()> buf;
+  int digits = count_digits(num);
+  int padding_needed = width - digits;
+
+  for (int _ = 0; _ < padding_needed; _++) {
+    RET_IF_RESULT_NEGATIVE(writer->write(padding));
+  }
+
+  return writer->write(*num_to_strview(num, buf));
+}
+
+} // namespace details
+
+namespace iso {
+
+/* Nonzero if YEAR is a leap year (every 4 years,
+   except every 100th isn't, and every 400th is).  */
+LIBC_INLINE bool is_leap(int year) {
+  return ((year % 4 == 0) && (year % 100 != 0 || year % 400 == 0));
+}
+
+static int iso_week_days(int yday, int wday) {
+  /* Add enough to the first operand of % to make it nonnegative.  */
+  int big_enough_multiple_of_7 = (-YDAY_MINIMUM / 7 + 2) * 7;
+  return (yday - (yday - wday + ISO_WEEK1_WDAY + big_enough_multiple_of_7) % 7 +
+          ISO_WEEK1_WDAY - ISO_WEEK_START_WDAY);
+}
+
+enum class IsoData {
+  GET_DATE,
+  GET_YEAR,
+};
+
+template <IsoData get_date_or_year>
+LIBC_INLINE int convert_iso(const FormatSection &to_conv) {
+  int year = to_conv.time->tm_year + YEAR_BASE;
+  int days = iso_week_days(to_conv.time->tm_yday, to_conv.time->tm_wday);
+
+  if (days < 0) {
+    /* This ISO week belongs to the previous year.  */
+    year--;
+    days = iso_week_days(to_conv.time->tm_yday + (365 + is_leap(year)),
+                       ...
[truncated]

jhuber6

Few comments between talks, I have never used strftime much so I can't say too much specifically.

jhuber6 · 2024-10-23T18:27:07Z

libc/src/time/strftime_core/CMakeLists.txt

+    .converter
+    libc.src.stdio.printf_core.writer
+    libc.include.time
+)


Missing newline.

jhuber6 · 2024-10-23T18:29:35Z

libc/src/time/strftime_core/core_structs.h

+  char padding;
+};
+
+#define RET_IF_RESULT_NEGATIVE(func)                                           \


Not a fan of macros like this, I don't think we have any others, but I get it being kind of annoying.

jhuber6 · 2024-10-23T18:30:19Z

libc/src/time/strftime_core/num_converter.h

+  return IntegerToString<uintmax_t>::format_to(bufref, num);
+}
+
+template <typename T> int count_digits(T num) {


We already have code for this, check integer_to_string.

michaelrj-google

Sorry it took me a while to get back to you on this. I've left a bunch of comments, feel free to ping me if you have questions.

michaelrj-google · 2024-11-12T19:02:03Z

libc/src/time/CMakeLists.txt

+# if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+#   add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+# endif()


why was this removed?

michaelrj-google · 2024-11-12T19:02:53Z

libc/src/time/CMakeLists.txt

+    libc.include.locale
+    libc.src.time.strftime_core.strftime_main
+    libc.src.stdio.printf_core.writer
+)


nit: missing a trailing newline

michaelrj-google · 2024-11-12T19:10:26Z

libc/src/time/strftime_core/core_structs.h

+  bool isE{false};
+  bool isO{false};


These should be stored with an enum like LengthModifier. Additionally, the standard mentions two flag characters that need to be stored, similar to printf (0 and +), see: https://pubs.opengroup.org/onlinepubs/9799919799/functions/strftime.html

michaelrj-google · 2024-11-12T19:12:58Z

libc/src/time/strftime_core/parser.h

+      section.has_conv = true;
+      section.time = &time;
+      ++cur_pos;
+      // locale-specific modifiers


Before you parse the format name you need to handle the flags and min width. For examples of what format specifiers might look like see: https://pubs.opengroup.org/onlinepubs/9799919799/functions/strftime.html#:~:text=year.%20For%20example%3A-,Year,-Conversion%20Specification

michaelrj-google · 2024-11-12T19:13:38Z

libc/src/time/strftime_core/str_converter.h

+int write_str(printf_core::Writer *writer, const FormatSection &to_conv) {
+  cpp::string_view str;
+  auto &time = *to_conv.time;
+  switch (to_conv.conv_name) {
+  case 'a':
+    str = safe_abbreviated_day_name(time.tm_wday);
+    break;
+  case 'A':
+    str = safe_day_name(time.tm_wday);
+    break;
+  case 'b':
+    str = safe_abbreviated_month_name(time.tm_mon);
+    break;
+  case 'B':
+    str = safe_month_name(time.tm_mon);
+    break;
+  case 'p':
+    str = to_conv.time->tm_hour >= 12 ? default_PM_str : default_AM_str;
+    break;
+  case 'z':
+    str = default_timezone_offset;
+    break;
+  case 'Z':
+    str = default_timezone_name;
+    break;
+  default:
+    return writer->write(to_conv.raw_string);
+  }
+  return writer->write(str);
+}


why is this separate from the main switch in converter.cpp?

michaelrj-google · 2024-11-12T19:24:46Z

libc/src/time/strftime_core/strftime_main.h

@@ -0,0 +1,32 @@
+//===-- Starting point for strftime -------------------------------*- C++


nit: fix the header formatting

michaelrj-google · 2024-11-12T19:24:53Z

libc/src/time/strftime_core/time_internal_def.h

@@ -0,0 +1,77 @@
+//===-- Strftime related internals -------------------------*- C++ -*-===//


nit: fix the header formatting

michaelrj-google · 2024-11-12T19:27:00Z

libc/test/src/time/CMakeLists.txt

+    strftime_test.cpp
+  DEPENDS
+    libc.src.time.strftime_core.strftime_main
+)


nit: fix missing newline

michaelrj-google · 2024-11-12T19:27:42Z

libc/test/src/time/strftime_core/parser_test.cpp

@@ -0,0 +1,102 @@
+//===-- Unittests for the printf Converter --------------------------------===//
+//


nit: fix this license header

michaelrj-google · 2024-11-12T19:28:11Z

libc/test/src/time/strftime_test.cpp

@@ -0,0 +1,113 @@
+


nit: remove this extra newline. Also fix the name on the first line of the license header

nickdesaulniers · 2024-11-14T17:29:43Z

libc/src/time/strftime.cpp

+                              strftime_core::overflow_write_mock, nullptr);
+  printf_core::Writer writer(&wb);
+  int ret = strftime_core::strftime_main(&writer, format, timeptr);
+  if (buffsz > 0) // if the buffsz is 0 the buffer may be a null pointer.


Perhaps before doing any work, we should explicitly validate the inputs? Then we could explicit check that the pointer parameters are not null, bufsz is not zero, etc.

michaelrj-google · 2025-01-07T18:04:19Z

Since there hasn't been progress on this patch in a while, I'm going to take over implementing strftime.

michaelrj-google · 2025-01-08T22:48:14Z

libc/src/time/strftime_core/num_converter.h

+LIBC_INLINE bool is_leap(int year) {
+  return ((year % 4 == 0) && (year % 100 != 0 || year % 400 == 0));
+}
+
+static int iso_week_days(int yday, int wday) {
+  /* Add enough to the first operand of % to make it nonnegative.  */
+  int big_enough_multiple_of_7 = (-YDAY_MINIMUM / 7 + 2) * 7;
+  return (yday - (yday - wday + ISO_WEEK1_WDAY + big_enough_multiple_of_7) % 7 +
+          ISO_WEEK1_WDAY - ISO_WEEK_START_WDAY);
+}
+
+enum class IsoData {
+  GET_DATE,
+  GET_YEAR,
+};
+
+template <IsoData get_date_or_year>
+LIBC_INLINE int convert_iso(const FormatSection &to_conv) {
+  int year = to_conv.time->tm_year + YEAR_BASE;
+  int days = iso_week_days(to_conv.time->tm_yday, to_conv.time->tm_wday);
+
+  if (days < 0) {
+    /* This ISO week belongs to the previous year.  */
+    year--;
+    days = iso_week_days(to_conv.time->tm_yday + (365 + is_leap(year)),
+                         to_conv.time->tm_wday);
+  } else {
+    int d = iso_week_days(to_conv.time->tm_yday - (365 + is_leap(year)),
+                          to_conv.time->tm_wday);
+    if (0 <= d) {
+      /* This ISO week belongs to the next year.  */
+      year++;
+      days = d;
+    }
+  }


This section of code is copied from glibc: https://github.com/bminor/glibc/blob/master/time/strftime_l.c#L383

That's not acceptable both for licensing and for plagiarism reasons. Copying code from other projects - even open source ones - can cause significant legal issues. The license glibc uses is called the "LGPL" and the license LLVM uses is based on the "Apache License". I'd recommend at least researching both of these before making any more PRs.

tszhin-swe · 2025-01-09T06:53:15Z

Sorry I missed that, I would make sure to be clear about licensing issues before making other PRs.

…

On Thu, 9 Jan 2025 at 7:26 AM, Michael Jones ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In libc/src/time/strftime_core/num_converter.h <#111305 (comment)>: > +LIBC_INLINE bool is_leap(int year) { + return ((year % 4 == 0) && (year % 100 != 0 || year % 400 == 0)); +} + +static int iso_week_days(int yday, int wday) { + /* Add enough to the first operand of % to make it nonnegative. */ + int big_enough_multiple_of_7 = (-YDAY_MINIMUM / 7 + 2) * 7; + return (yday - (yday - wday + ISO_WEEK1_WDAY + big_enough_multiple_of_7) % 7 + + ISO_WEEK1_WDAY - ISO_WEEK_START_WDAY); +} + +enum class IsoData { + GET_DATE, + GET_YEAR, +}; + +template <IsoData get_date_or_year> +LIBC_INLINE int convert_iso(const FormatSection &to_conv) { + int year = to_conv.time->tm_year + YEAR_BASE; + int days = iso_week_days(to_conv.time->tm_yday, to_conv.time->tm_wday); + + if (days < 0) { + /* This ISO week belongs to the previous year. */ + year--; + days = iso_week_days(to_conv.time-> tm_yday + (365 + is_leap(year)), + to_conv.time->tm_wday); + } else { + int d = iso_week_days(to_conv.time->tm_yday - (365 + is_leap(year)), + to_conv.time->tm_wday); + if (0 <= d) { + /* This ISO week belongs to the next year. */ + year++; + days = d; + } + } This section of code is copied from glibc: https://github.com/bminor/glibc/blob/master/time/strftime_l.c#L383 That's not acceptable both for licensing and for plagiarism reasons. Copying code from other projects - even open source ones - can cause significant legal issues. The license glibc uses is called the "LGPL" and the license LLVM uses is based on the "Apache License". I'd recommend at least researching both of these before making any more PRs. — Reply to this email directly, view it on GitHub <#111305 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BJM5SASTSKNAK5FBON6FOQ32JWX2HAVCNFSM6AAAAABPOP7M7OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMZYGQZDOOBQHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

TODO: DESCRIPTION TODO: TESTS TODO: RotFO (Rest of the Formatting Options) Roughly based on llvm#111305, but with major rewrites.

Implements the posix-specified strftime conversions for the default locale, along with comprehensive unit tests. This reuses a lot of design from printf, as well as the printf writer. Roughly based on #111305, but with major rewrites.

Implements the posix-specified strftime conversions for the default locale, along with comprehensive unit tests. This reuses a lot of design from printf, as well as the printf writer. Roughly based on llvm#111305, but with major rewrites.

graphite-app bot reviewed Oct 6, 2024

View reviewed changes

tszhin-swe and others added 2 commits October 13, 2024 21:59

[libc] Implement strftime

560e57a

[libc] Add stubs for 'strftime' function

8ea3b4c

Summary: This patch does not actually implement the function, but provides the interface. This partially resovles llvm#106630 as it will allow us to build libc++ with locale support (so long as you don't use it).

tszhin-swe force-pushed the 1002Blank branch from 2f06475 to b00a698 Compare October 13, 2024 15:39

graphite-app bot reviewed Oct 13, 2024

View reviewed changes

tszhin-swe changed the title ~~[WIP] Basic structures for strftime~~ [libc] Implement strftime Oct 13, 2024

tszhin-swe force-pushed the 1002Blank branch from b00a698 to 96694d2 Compare October 16, 2024 17:11

[libc] Implement strftime and strftime_l

097d459

tszhin-swe force-pushed the 1002Blank branch from 96694d2 to 097d459 Compare October 19, 2024 16:09

[libc] Refactor converter.cpp

c4f5728

tszhin-swe force-pushed the 1002Blank branch from e036872 to c4f5728 Compare October 23, 2024 14:30

tszhin-swe marked this pull request as ready for review October 23, 2024 14:31

llvmbot added the libc label Oct 23, 2024

jhuber6 reviewed Oct 23, 2024

View reviewed changes

jhuber6 requested review from frobtech and michaelrj-google October 23, 2024 18:31

michaelrj-google reviewed Nov 12, 2024

View reviewed changes

nickdesaulniers reviewed Nov 14, 2024

View reviewed changes

michaelrj-google closed this Jan 7, 2025

michaelrj-google reviewed Jan 8, 2025

View reviewed changes

michaelrj-google added a commit to michaelrj-google/llvm-project that referenced this pull request Jan 11, 2025

[libc] Implement strftime

9e875ec

TODO: DESCRIPTION TODO: TESTS TODO: RotFO (Rest of the Formatting Options) Roughly based on llvm#111305, but with major rewrites.

michaelrj-google mentioned this pull request Jan 11, 2025

[libc] Implement strftime #122556

Merged

		const FormatSection &to_conv) {
		return details::write_num<1>(to_conv.time->tm_wday == 0 ? 7 : to_conv.time->tm_wday, writer);

		@@ -0,0 +1,32 @@
		//===-- Starting point for strftime -------------------------------*- C++

		@@ -0,0 +1,77 @@
		//===-- Strftime related internals -------------------------- C++ --===//

		@@ -0,0 +1,102 @@
		//===-- Unittests for the printf Converter --------------------------------===//
		//

[libc] Implement strftime #111305

[libc] Implement strftime #111305

Uh oh!

Conversation

tszhin-swe commented Oct 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot Oct 6, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Oct 23, 2024

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelrj-google commented Jan 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tszhin-swe commented Jan 9, 2025 via email

Uh oh!

Uh oh!

tszhin-swe commented Oct 6, 2024 •

edited

Loading

github-actions bot commented Oct 6, 2024 •

edited

Loading