Re: Allow embedded timestamps by C/C++ macros to be set externally (3)

[Dhole <dhole at openmailbox dot org>]

Hi Bernd,

On 16-04-25 12:15:50, Bernd Schmidt wrote:
> On 04/18/2016 02:26 PM, Dhole wrote:
> >A few months ago I submited a patch to allow the embedded timestamps by
> >C/C++ macros to be set externally [2], which was already an improvement
> >over [1].  I was told to wait until the GCC 7 stage 1 started to send
> >this patch again.
> 
> >+/* Read SOURCE_DATE_EPOCH from environment to have a deterministic
> >+   timestamp to replace embedded current dates to get reproducible
> >+   results. Returns -1 if SOURCE_DATE_EPOCH is not defined.  */
> >+long long
> >+get_source_date_epoch()
> 
> Always have a space before open-paren. Maybe this should return time_t. See
> below.
> 
> >+/* Read SOURCE_DATE_EPOCH from environment to have a deterministic
> >+   timestamp to replace embedded current dates to get reproducible
> >+   results. Returns -1 if SOURCE_DATE_EPOCH is not defined.  */
> >+extern long long get_source_date_epoch();
> 
> Double space after the end of a sentence. Space before open paren.
> 
> >+  source_date_epoch = get_source_date_epoch();
> >+  cpp_init_source_date_epoch(parse_in, source_date_epoch);
> 
> Spaces.
> 
> >+/* Initialize the source_date_epoch value.  */
> >+extern void cpp_init_source_date_epoch (cpp_reader *, long long);
> 
> Also thinking we should be using time_t here.
> 
> >  /* Sanity-checks are dependent on command-line options, so it is
> >     called as a subroutine of cpp_read_main_file ().  */
> 
> We don't write () to mark function names.
> 
> >+	     tb = gmtime ((time_t*) &pfile->source_date_epoch);
> 
> Space before the "*". But this cast looks ugly and unreliable (think
> big-endian). This is why I would prefer to move to a time_t representation
> sooner.
> 
> >2016-04-18  Eduard Sanou<dhole@openmailbox.org>
> >	    Matthias Klose<doko@debian.org>
> >	* c-common.c (get_source_date_epoch): New function, gets the environment
> >	variable SOURCE_DATE_EPOCH and parses it as long long with error
> >	handling.
> >	* c-common.h (get_source_date_epoch): Prototype.
> >	* c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch.
> 
> Add blank lines after the end of the names in ChangeLogs.

Thanks for the review!

I've fixed all the spaces issues.  I've also changed the "unsigned long long"
to "time_t" as you suggested.  Also, to improve reliability I'm now
using strtoll rather than strtoull, so that negative values can be
detected in SOURCE_DATE_EPOCH, which are treated as errors.  This way
the variable pfile->source_date_epoch can't be set to -1 (which is the
default value) when SOURCE_DATE_EPOCH is defined.

I'm attaching the improved patch with the ChangeLog.

Cheers,
-- 
Dhole

Attachment: gcc-SOURCE_DATE_EPOCH-patch-2016_04_26.diff.txt
Description: Text document

Attachment: ChangeLog
Description: Text document

Attachment: signature.asc
Description: PGP signature