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

Reply via email to