[libcxx-commits] [PATCH] D124175: [libcxx] Reject month 0 in get_date/__get_month

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!

In D124175#3464773 <https://reviews.llvm.org/D124175#3464773>, @DavidSpickett wrote:

> For https://github.com/llvm/llvm-project/issues/47663.
>
> From https://en.cppreference.com/w/cpp/locale/time_get/get_date (not sure if I should be getting the actual standard document for this) this seems like it could be ok.

Nothing wrong with cppreference, but for the Standard wording we usually use http://eel.is/c++draft/locale.time.get.virtuals#lib:do_get_date,time_get. (Not sure whether you know this resource.)

> I read in a draft standard for `do_get_date` specifically "Returns: An iterator pointing immediately beyond the last character recognized as possibly part of a valid date.". So "possibly" seems like it would allow for both behaviours.

I think libstdc++'s behaviour is the correct behaviour. But fixing that should be a separate review.



================
Comment at: libcxx/test/std/localization/locale.categories/category.time/locale.time.get.byname/get_date.pass.cpp:18
 // GLIBC fails on the zh_CN test.
 // XFAIL: linux
 
----------------
:-( This is not your change, but it basically means these changes aren't tested in most of our CI. I'll create a fix.


================
Comment at: libcxx/test/std/localization/locale.categories/category.time/locale.time.get.byname/get_date.pass.cpp:114
+        // We consume the first two chars checking the month.
+        assert(base(i) == in+2);
+        // tm is not modified.
----------------
This should be guarded with
```
#if _LIBCPP_VERSION
        assert(base(i) == in+2);
#else
        assert(base(i) == in+2);
#endif
```
Our tests should be portable and MSVC STL and GCC use them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124175/new/

https://reviews.llvm.org/D124175