Re: RFR: 8247781: Day periods support

[Stephen Colebourne]

On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato  wrote:

> Hi,
> 
> Please review the changes for the subject issue. This is to enhance the 
> java.time package to support day periods, such as "in the morning", defined 
> in CLDR. It will add a new pattern character 'B' and its supporting builder 
> method. The motivation and its spec are in this CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8254629
> 
> Naoto

Changes requested by scolebourne (Author).

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
981:

> 979: 
> 980: {"B", "Text(DayPeriod,SHORT)"},
> 981: {"BB", "Text(DayPeriod,SHORT)"},

"BB" and "BBB" are not defined to do anything in the CSR. Java should match 
CLDR/LDML rules here.

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
540:

> 538: builder.appendDayPeriodText(TextStyle.FULL);
> 539: DateTimeFormatter f = builder.toFormatter();
> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");

This should be "DayPeriod(FULL)", because an end user might create a 
`TemporalField` with the name "DayPeriod" and the toString should be unique.

src/java.base/share/classes/java/time/format/Parsed.java line 352:

> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ? 
> fieldValues.get(MINUTE_OF_HOUR) : 0);
> 351: if (!dayPeriod.includes(mod)) {
> 352: throw new DateTimeException("Conflict found: " + 
> changeField + " conflict with day period");

"conflict with day period" -> "conflicts with day period"

Should also include `changeValue` and ideally the valid range

src/java.base/share/classes/java/time/format/Parsed.java line 472:

> 470: }
> 471: if (dayPeriod != null) {
> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {

Are we certain that the CLDR data does not contain day periods based on minutes 
as well as hours? This logic does not check against MINUTE_OF_HOUR for example. 
The logic also conflicts with the spec Javadoc that says MINUTE_OF_HOUR is 
validated.

src/java.base/share/classes/java/time/format/Parsed.java line 497:

> 495: AMPM_OF_DAY.checkValidValue(ap);
> 496: }
> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 
> 720);

No need to put `AMPM_OF_DAY` back in here because you've already resolved it to 
`HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be validation 
to check that the day period agrees with the AM/PM value.

src/java.base/share/classes/java/time/format/Parsed.java line 500:

> 498: }
> 499: }
> 500: }

Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored if 
there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check 
`AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and 
HOUR_OF_DAY=18 does not look like it throws an exception, when it probably 
should).

On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line 
427, checking for conflicts with any parsed day period. (a small bug fix 
behavioural change)

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
1489:

> 1487: Objects.requireNonNull(style, "style");
> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT && 
> style != TextStyle.NARROW) {
> 1489: throw new IllegalArgumentException("Style must be either 
> full, short, or narrow");

Nothing in the Javadoc describes this behaviour. It would make more sense to 
map FULL_STANDALONE to FULL and so on and not throw an exception.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
1869:

> 1867: } else if (cur == 'B') {
> 1868: switch (count) {
> 1869: case 1, 2, 3 -> 
> appendDayPeriodText(TextStyle.SHORT);

I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I 
don't think they are in LDML. I note that patterns G and E do this though, so 
there is precedent.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5094:

> 5092: @Override
> 5093: public String toString() {
> 5094: return "Text(DayPeriod," + textStyle + ")";

Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the text 
printer/parser

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5153:

> 5151:  * minute-of-day of "at" or "from" attribute
> 5152:  */
> 5153: private final long from;

Could these three instance variables be `int` ?

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5252:

> 5250: 
> .getLocaleResources(CalendarDataUtility.findRegionOverride(l));
> 5251: String dayPeriodRules = lr.getRules()[1];
> 5252: final 

Re: Windows: Replace VisualStudio with gcc/clang?

[Stephen Colebourne]

On 12 March 2018 at 15:36, Erik Joelsson  wrote:
> On 2018-03-12 03:18, Magnus Ihse Bursie wrote:
> Just to comment on this. Microsoft has introduced standalone C++ build
> tools:
>
> http://landinghub.visualstudio.com/visual-cpp-build-tools
>
> I haven't looked into it yet, but it seems to be a much more lightweight
> alternative to installing Visual Studio if you just want to be able to build
> something. We should certainly make sure this works when full support for VS
> 2017 is in.

>From my perspective, something needs to change, as I found it
completely impossible to install the old Visual Studio releases the
JDK demands (and it took ages just to find them). Requiring outdated
tooling to build the JDK really isn't great.

Stephen

README in jdk8/tl root

[Stephen Colebourne]

The README lists 6 repositories needed, whereas there are now 7.
Nashorn is missing.
http://hg.openjdk.java.net/jdk8/tl/file/28be3d174c92/README


Stephen