Re: [v3 PATCH 00/11] Pull Request - changelog

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 16, 2018 at 04:35:47PM -0500, J William Piggott wrote:
> > I hope I'll work on cal(1) this week. If you see any bug in the
> > current code, then fix it again the current master branch. I'll rebase
> > and merge it to the new code later... or wait ;-)
> 
> That's not cool Karel. You requested volunteers to do this for you. I've been
> working on it for nearly a month, and now you want to pull it out from under me.

Ah, I don't want to say I'll ignore your patches or so. This is
misunderstanding... anyway merged!

> I'm submitting 2 patches that implement exactly what you've asked for; all by
> the book; nothing removed, no short options, etc. Plus one patch to update the
> man page. If there is something wrong with the submission I'd like some
> constructive feedback so that I can fix it.

Yes, sounds good. Thanks! (and thanks for patience with stubborn
maintainer;-)


I'm not sure about the way how the current cal(1) code handles the
reform. It's all based on "year", but it could be more complex if we
want to make it extendable and support another reforms too. This is
what I'd like to change.

The ideal would be to describe the reform by any struct (as good
programming is about data structures rather than about code:-).


struct cal_reform {
    int year;
    int month;
    int day;
    int missing_days;
};

static const struct reforms[] = {
    [REFORM_GB_1752] = { 
        .year = 1752,
        .month = SEPTEMBER,
        .day = 2,
        .missing_days = 11
    },
    [REFORM_GREEK_1923] = {
        .year = 1923,
        .month = FEBRUARY,
        .day = 16,
        .missing_days = 12
    }
    ...
};

/*
 * Names and aliases used in --help output and for getopt()
 */
struct cal_reform_names {
    const char *name;
    int id;
    const char *desc;
};

static const struct reform_names[] = {
    { "1752", REFORM_GB_1752, _N("British Empire reform (September 1752)")_ },
    ...
};


main():
    ctl.reform = &reforms[REFORM_GB_1752];
...


day_in_week():
    struct cal_reform *re = ctl->reform;

	if (re->year < year
	    || (year == re->year && re->month < month)
	    || (year == re->year
		&& month == re->month && re->day + re->missing_days < day)) {

        ...
	}

Comments? 

IMHO it's better and more readable than the current hardcoded stuff.


The remaining thing is magic constants for day-in-week calculations.
It's fast/elegant for Gregorian (ISO) calendar, but I'm not sure about
old calendars. 

Maybe the original calculation before 91ac9ef5db449b38e679e85483f2dc6d9a56e1ce
has been more readable (based on leap_years_since_year_1() and easy to adopt 
to another calendars. Ideas?

    Karel


-- 
 Karel Zak  <kzak@xxxxxxxxxx>
 http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux