[libcxx-commits] [PATCH] D155364: [libc++][format] Improves compile-time diagnostics.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante created this revision.
Herald added a project: All.
Mordante published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

Then a std-format-spec option is invalid for a type the compile-time
validation will detect its usage and issue a diagnostic. Before it
validated after parsing the entire std-format-spec, which meant the
diagnostic was less precise. It would be possible to do this validation
run-time but that has a performance overhead. When using the format
family of functions, this would be unneeded overhead; the validation was
done at run-time. For the vformat family it would give better
diagnostics.

To avoid paying what you don't use, it has been decided to aim for the
better performance. It's more likely users will use the format family of
functions.

Depends on D155264 <https://reviews.llvm.org/D155264>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155364

Files:
  libcxx/include/__format/parser_std_format_spec.h


Index: libcxx/include/__format/parser_std_format_spec.h
===================================================================
--- libcxx/include/__format/parser_std_format_spec.h
+++ libcxx/include/__format/parser_std_format_spec.h
@@ -32,6 +32,7 @@
 #include <__iterator/iterator_traits.h> // iter_value_t
 #include <__memory/addressof.h>
 #include <__type_traits/common_type.h>
+#include <__type_traits/is_constant_evaluated.h>
 #include <__type_traits/is_trivially_copyable.h>
 #include <__variant/monostate.h>
 #include <cstdint>
@@ -307,6 +308,19 @@
 template <class _CharT>
 class _LIBCPP_TEMPLATE_VIS __parser {
 public:
+  // Parses the format specification.
+  //
+  // Depending on whether the parsing is done compile-time or run-time
+  // the method slightly differs.
+  // - Only parses a field when it is in the __fields. Accepting all
+  //   fields and then validate the valid ones has a performance impact.
+  //   This is faster but gives slighly worse error messages.
+  // - compile-time when a field is not accepted the parser will still
+  //   parse it and give an error when it's present. This gives a more
+  //   accurate error.
+  // The idea is that most times the format instead of the vformat
+  // functions are used. In that case the error will be detected during
+  // compilation and there is no need to pay for the run-time overhead.
   template <class _ParseContext>
   _LIBCPP_HIDE_FROM_ABI constexpr typename _ParseContext::iterator __parse(_ParseContext& __ctx, __fields __fields) {
     auto __begin = __ctx.begin();
@@ -317,26 +331,45 @@
     if (__parse_fill_align(__begin, __end, __fields.__use_range_fill_) && __begin == __end)
       return __begin;
 
-    if (__fields.__sign_ && __parse_sign(__begin) && __begin == __end)
-      return __begin;
+    if (__fields.__sign_) {
+      if (__parse_sign(__begin) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_sign(__begin)) {
+      std::__throw_format_error("The format specification does not allow the sign option");
+    }
 
-    if (__fields.__alternate_form_ && __parse_alternate_form(__begin) && __begin == __end)
-      return __begin;
+    if (__fields.__alternate_form_) {
+      if (__parse_alternate_form(__begin) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_alternate_form(__begin))
+      std::__throw_format_error("The format specifier does not allow the alternate form option");
 
-    if (__fields.__zero_padding_ && __parse_zero_padding(__begin) && __begin == __end)
-      return __begin;
+    if (__fields.__zero_padding_) {
+      if (__parse_zero_padding(__begin) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_zero_padding(__begin))
+      std::__throw_format_error("The format specifier does not allow the zero-padding option");
 
     if (__parse_width(__begin, __end, __ctx) && __begin == __end)
       return __begin;
 
-    if (__fields.__precision_ && __parse_precision(__begin, __end, __ctx) && __begin == __end)
-      return __begin;
-
-    if (__fields.__locale_specific_form_ && __parse_locale_specific_form(__begin) && __begin == __end)
-      return __begin;
-
-    if (__fields.__clear_brackets_ && __parse_clear_brackets(__begin) && __begin == __end)
-      return __begin;
+    if (__fields.__precision_) {
+      if (__parse_precision(__begin, __end, __ctx) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_precision(__begin, __end, __ctx))
+      std::__throw_format_error("The format specifier does not allow the precision option");
+
+    if (__fields.__locale_specific_form_) {
+      if (__parse_locale_specific_form(__begin) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_locale_specific_form(__begin))
+      std::__throw_format_error("The format specifier does not allow the locale-specific form option");
+
+    if (__fields.__clear_brackets_) {
+      if (__parse_clear_brackets(__begin) && __begin == __end)
+        return __begin;
+    } else if (std::is_constant_evaluated() && __parse_clear_brackets(__begin))
+      std::__throw_format_error("The format specifier does not allow the n option");
 
     if (__fields.__type_)
       __parse_type(__begin);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D155364.540663.patch
Type: text/x-patch
Size: 4374 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230715/5f881547/attachment.bin>