On Thu, Apr 28, 2016 at 11:20:28AM +0200, Matthias Klose wrote:
> On 27.04.2016 17:56, Dhole wrote:
> >Thanks again for the review Bernd,
> >
> >On 16-04-27 01:33:47, Bernd Schmidt wrote:
> >>>+ epoch = strtoll (source_date_epoch, &endptr, 10);
> >>>+ if ((errno == ERANGE && (epoch == LLONG_MAX || epoch == LLONG_MIN))
> >>>+ || (errno != 0 && epoch == 0))
> >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable
> >>>$SOURCE_DATE_EPOCH: "
> >>>+ "strtoll: %s\n", xstrerror(errno));
> >>>+ if (endptr == source_date_epoch)
> >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable
> >>>$SOURCE_DATE_EPOCH: "
> >>>+ "No digits were found: %s\n", endptr);
> >>>+ if (*endptr != '\0')
> >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable
> >>>$SOURCE_DATE_EPOCH: "
> >>>+ "Trailing garbage: %s\n", endptr);
> >>>+ if (epoch < 0)
> >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable
> >>>$SOURCE_DATE_EPOCH: "
> >>>+ "Value must be nonnegative: %lld \n", epoch);
> >>
> >>These are somewhat unusual for error messages, but I think the general
> >>principle of no capitalization probably applies, so "No", "Trailing", and
> >>"Value" should be lowercase.
> >
> >Done.
> >
> >>>+ time_t source_date_epoch = (time_t) -1;
> >>>+
> >>>+ source_date_epoch = get_source_date_epoch ();
> >>
> >>First initialization seems unnecessary. Might want to merge the declaration
> >>with the initialization.
> >
> >And done.
> >
> >I'm attaching the updated patch with the two minor issues fixed.
>
> committed.
BTW, I think fatal_error doesn't make sense, it isn't something that is not
recoverable, normal error or just a warning would be IMHO more than
sufficient. The fallback would be just using current time, i.e. ignoring
the env var.
Additionally, I think it is a very bad idea to slow down the initialization
for something so rarely used - instead of initializing this always, IMNSHO
it should be only initialized when the first __TIME__ macro is expanded,
similarly how it only calls time/localtime etc. when the macro is expanded
for the first time.
Also, as a follow-up, guess the driver should set this
env var for the -fcompare-debug case if not already set, to something that
matches the current date, so that __TIME__ macros expands the same in
between both compilations, even when they don't compile both in the same
second.
Jakub
