Bug 250802 - Remove duplicate isLeapYear
Summary: Remove duplicate isLeapYear
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ahmad Saleem
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-01-18 15:54 PST by Ahmad Saleem
Modified: 2023-01-19 05:37 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ahmad Saleem 2023-01-18 15:54:54 PST
Hi Team,

I came across another refactoring patch to remove duplicate function:

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/ecbe8115cc1f014eb61dff29b96e405a0ab3ef51

WebKit Source - https://searchfox.org/wubkat/source/Source/WTF/wtf/DateMath.h#188 & https://searchfox.org/wubkat/source/Source/WebCore/platform/DateComponents.cpp#56

We have two functions exactly same, one is inline while another is static.

Can we get rid of one?

Thanks!
Comment 1 Brent Fulgham 2023-01-18 16:23:47 PST
Yes! Use the one in WTF/DateMath.h and delete the other one.
Comment 2 Alexey Proskuryakov 2023-01-18 16:35:47 PST
It would be nice to move other generic functions as well, and unify duplicates (like daysInMonths in ISO8601.cpp). But that would be a much larger effort.

static int maxDayOfMonth(int year, int month)
static int dayOfWeek(int year, int month, int day)
Comment 3 Ahmad Saleem 2023-01-18 16:36:59 PST
(In reply to Alexey Proskuryakov from comment #2)
> It would be nice to move other generic functions as well, and unify
> duplicates (like daysInMonths in ISO8601.cpp). But that would be a much
> larger effort.
> 
> static int maxDayOfMonth(int year, int month)
> static int dayOfWeek(int year, int month, int day)

Should I create a bug for future?
Comment 4 Ahmad Saleem 2023-01-18 16:37:12 PST
(In reply to Brent Fulgham from comment #1)
> Yes! Use the one in WTF/DateMath.h and delete the other one.

Happy to do PR. :-)
Comment 5 Alexey Proskuryakov 2023-01-18 16:47:27 PST
> Should I create a bug for future?

I personally wouldn't bother (refactoring ideas don't usually get acted on), but it wouldn't be unreasonable to do if you feel like it.
Comment 6 Ahmad Saleem 2023-01-18 16:54:12 PST
(In reply to Alexey Proskuryakov from comment #5)
> > Should I create a bug for future?
> 
> I personally wouldn't bother (refactoring ideas don't usually get acted on),
> but it wouldn't be unreasonable to do if you feel like it.

Not in near future (few weeks), still learning few bits around WebKit but in few months, I can explore it as a challenge. I will add it in my To-do Notes. :-)
Comment 7 Ahmad Saleem 2023-01-18 17:07:57 PST
PR - https://github.com/WebKit/WebKit/pull/8806
Comment 8 EWS 2023-01-19 05:36:50 PST
Committed 259080@main (0b9c23b10f38): <https://commits.webkit.org/259080@main>

Reviewed commits have been landed. Closing PR #8806 and removing active labels.
Comment 9 Radar WebKit Bug Importer 2023-01-19 05:37:16 PST
<rdar://problem/104425042>