[Mono-dev] : DateTime.Parse patch

[Eyal Alaluf]

Hi, Atsushi.

The eu-ES locale has a wrong MonthDayPattern. Its 'MMMM' instead of
'MMMM dd'. This is why I added this line.
How do you change the locale data?

The 'valuePos >= ' occurred during development (I think it was some
white space skipping). I'll run a pass at minimizing these checks.

I'll remove the comment about the bug-fix and add a test case for this.

Thanks for the comments, Eyal.

-----Original Message-----
From: mono-devel-list-bounces at lists.ximian.com
[mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf Of Atsushi
Eno
Sent: 21 August 2007 10:31
To: Eyal Alaluf
Cc: mono-devel-list at lists.ximian.com
Subject: Re: [Mono-dev] : DateTime.Parse patch

Hi,

Comments part by part (as you have attached the patch as octet-stream
I cannot simply quote them like others' patches):

+			int dayIndex = dfi.MonthDayPattern.IndexOf('d');
+			int monthIndex =
dfi.MonthDayPattern.IndexOf('M');
+			if (dayIndex == -1)
+				return true; // BUGBUG: Hack for wrong
pattern in Locale eu-ES

I wonder how you needed this line.

+		internal static bool _ParseDateSeparator (string s, int
sPos, 
DateTimeFormatInfo dfi, bool exact, out int num_parsed)

Some of new methods do not have to be accessible as internal.

-				if (s.Length == valuePos)
+				if (valuePos >= s.Length)

(Not limited to this specific change) why did you need this kind
of changes? Is there such a case that cause valuePos could become
bigger than s.Length, or are you ignoring error-prone cases?

-						ZeroPad (result,
dfi.Calendar.GetYear (this), (tokLen == 3 ? 3 : 4));
+						//bug fix:
+						//Unitest:(new DateTime
(1999,5,4)).ToString ("yyyyyyyyyy") 
should return "0000001999"
+						ZeroPad (result,
dfi.Calendar.GetYear (this), tokLen);

What do you mean here?

And in general as a whole patch please check our coding styles:
http://www.mono-project.com/Coding_Guidelines
(like foo () instead of foo())

Thanks a lot for the improvements :-)

Atsushi Eno

Eyal Alaluf wrote:
> Attached is a revised patch that takes into consideration the
> ShortDatePattern and MonthDayPattern of the DateTimeFormatInfo and
does
> not have anymore the 'switch' case on the culture ID.
> 
> Your comments are welcome, Eyal.
> 
> -----Original Message-----
> From: mono-devel-list-bounces at lists.ximian.com
> [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf Of Eyal
> Alaluf
> Sent: 16 August 2007 14:46
> To: Atsushi Eno
> Cc: mono-devel-list at lists.ximian.com
> Subject: Re: [Mono-dev] : DateTime.Parse patch
> 
> Hi, Atsushi.
> 
> Thanks to your remark I checked again and saw that MS behaviour is to
> decide upon the month day and year order according to the
> ShortDatePattern in all cases.
> When I wrote the code I thought that 'yyyy/MM/dd' didn't indicate the
> order but I was wrong.
> So actually, the switch is not necessary. Instead checking if 'd'
> appears before 'M' is enough.
> I actually went ahead and modified the
> culture.DateTimeFormat.ShortDatePattern and this modification overrode
> the decision if 1/10 is Oct 1st or Jan 10th.
> This brought up another complication since I didn't consider the order
> of the year according to this pattern and if it says 'yyyy/MM/dd' then
> '01/02/03' actually means '2001 Feb 3rd' and 'dd/MM/yyyy' would spit
> '2003 Feb 1st'.
> I am adding the year logic and will prepare next week a new patch.
> 
> Eyal.
> 
> -----Original Message-----
> From: mono-devel-list-bounces at lists.ximian.com
> [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf Of Atsushi
> Eno
> Sent: 16 August 2007 05:19
> To: Eyal Alaluf
> Cc: mono-devel-list at lists.ximian.com
> Subject: Re: [Mono-dev] : DateTime.Parse patch
> 
> Hello,
> 
> I like the effort to improve DateTime.Parse(), but I think your
> approach is broken. The switch-case that looks at 
> DateTimeFormatInfo.CultureID could be problematic especially
> when a DateTimeFormatInfo is cloned and then its format strings
> are modified (note that there are setters on DateTimeFormatInfo
> members).
> 
> Atsushi Eno
> 
> Eyal Alaluf wrote:
>> Hi, all.
>>
>> Attached is a patch for the infamous DateTime.Parse method.
>> The current logic for Parse is to try all the culture date time
> formats
>> and then try some special formats in invariant culture.
>> This solution is not sufficient because it is not very compatible
with
>> MS behavior, it is difficult to maintain and is very inefficient.
>>
>> The current patch aims to improve all of these issues. The patch
> defines
>> two sets of formats 'date' only formats and 'time' only formats.
>> The Parse method will check all the date formats for a match. If the
>> date format matches the string beginning it will combine it with all
> the
>> time formats. It will check similarly all the time formats and match
> to
>> them the date formats.
>> Only if this fails it will look at all the culture date formats
> (making
>> the TrypParse negative flow still very slow). This is done because
>> additional effort is required to define the culture specific
> separators.
>> The result is that many more formats are recognizable by
> DateTime.Parse
>> and that it is easier to add formats if necessary.
>>
>> In different flows the patch improves performance by a factor of 3-5
>> times on Mono.
>>
>> Please review and provide feedback since this is a fairly major
change
>> to a really sensitive spot.
>>
>> Eyal.
>>
>>
>>
>
------------------------------------------------------------------------
>> _______________________________________________
>> Mono-devel-list mailing list
>> Mono-devel-list at lists.ximian.com
>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
> 
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list

_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list