[libcxx-commits] [libcxx] [libc++][format] Don't treat a closing '}' as part of format-spec (PR #81305)

[Mark de Wever via libcxx-commits]

================
@@ -251,7 +251,7 @@ __handle_replacement_field(_Iterator __begin, _Iterator __end, _ParseCtx& __pars
   if (__r.__last == __end)
     std::__throw_format_error("The argument index should end with a ':' or a '}'");
 
-  bool __parse = *__r.__last == _CharT(':');
+  bool __parse = __parse_ctx.__should_parse() = *__r.__last == _CharT(':');
----------------
mordante wrote:

> After reading your comment and the Standard, I'm thinking maybe doing an early return just before calling __parse_fill_align is the right thing to do.

Yes testing for `_CharT('}')` before calling `__parse_fill_align` sounds like a cleaner solution my proposed solution.

> Specialized formatters (__validate_fill_character that is) shouldn't even have a say about that }, 

Correct it can still call out `{` since this is always invalid. Maybe we need to investigate ':' for range formatters in more detail. "{:::<<}" is probably rejected. I expect it should be an underlying formatter with left-alignment and `<` as fill character. (I have not tested that.) Maybe it would be good to investigate that for a follow-up patch.

> For example, we can parse {:}<} as having an empty format-spec followed by an invalid escape sequence rather than having an invalid fill followed by an align and a closing brace. The ill-fomed-ness doesn't change but the exception.what() does, so we need to fix the tests checking this if we are going this route.

Correct that is exactly what we should do.

> Does this concludes what you mean?

Yes.

https://github.com/llvm/llvm-project/pull/81305