Re: [PATCH 2/3] convert enum date_mode into a struct

[Junio C Hamano]

Jeff King p...@peff.net writes:

 ...  However, the tricky case is where we use the
 enum labels as constants, like:

   show_date(t, tz, DATE_NORMAL);

 Ideally we could say:

   show_date(t, tz, { DATE_NORMAL });

 but of course C does not allow that.
 ...
   3. Provide a wrapper that generates the correct struct on
  the fly. The big downside is that we end up pointing to
  a single global, which makes our wrapper non-reentrant.
  But show_date is already not reentrant, so it does not
  matter.

 This patch implements 3, along with a minor macro to keep
 the size of the callers sane.

Another big downside is that DATE_NORMAL is defined to be 0.

This makes it very cumbersome to merge a side branch that uses an
outdated definition of show_date() and its friends and tell them
to show date normally.  The compiler does not help detecting
places that need to be adjusted during merge and instead just pass
a NULL pointer as a pointer to the new struct.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[Jeff King]

On Tue, Jul 07, 2015 at 01:37:08PM -0700, Junio C Hamano wrote:

3. Provide a wrapper that generates the correct struct on
   the fly. The big downside is that we end up pointing to
   a single global, which makes our wrapper non-reentrant.
   But show_date is already not reentrant, so it does not
   matter.
 
  This patch implements 3, along with a minor macro to keep
  the size of the callers sane.
 
 Another big downside is that DATE_NORMAL is defined to be 0.
 
 This makes it very cumbersome to merge a side branch that uses an
 outdated definition of show_date() and its friends and tell them
 to show date normally.  The compiler does not help detecting
 places that need to be adjusted during merge and instead just pass
 a NULL pointer as a pointer to the new struct.

My assumption was that using the raw 0 is something we would frowned
upon in new code. There was a single historical instance that I fixed in
the series, but I wouldn't expect new ones (and actually, that instance
was 1, which would be caught by the compiler).

However, if you're concerned, I think we could have show_date massage a
NULL date, like:

diff --git a/date.c b/date.c
index 8f91569..a04d089 100644
--- a/date.c
+++ b/date.c
@@ -173,6 +173,10 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
 {
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
+   static const struct fallback_mode = { DATE_NORMAL };
+
+   if (!mode)
+   mode = fallback_mode;
 
if (mode-type == DATE_RAW) {
strbuf_reset(timebuf);


That would also allow people to explicitly call:

  show_date(t, tz, NULL);

to get the default format, though I personally prefer spelling it out.

I guess we _could_ introduce:

  #define DATE_MODE(x) ((struct date_mode *)(x))

and then take any numeric value, under the assumption that the first
page of memory will never be a valid pointer:

diff --git a/date.c b/date.c
index 8f91569..f388fee 100644
--- a/date.c
+++ b/date.c
@@ -173,6 +173,18 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
 {
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
+   struct date_mode fallback;
+
+   /* hysterical compatibility */
+   if (mode  1024) {
+   if (mode == DATE_STRFTIME)
+   die(BUG: nice try);
+   fallback.type = mode;
+   mode = fallback;
+   }
+
+   if (!mode)
+   mode = fallback_mode;
 
if (mode-type == DATE_RAW) {
strbuf_reset(timebuf);

That's kind of nasty, but at least it's hidden from the callers.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[Jeff King]

On Tue, Jul 07, 2015 at 02:05:52PM -0700, Junio C Hamano wrote:

 And that is because DATE_NORMAL is defined to be 0; we can claim
 that the compiler is being stupid to take one of the enum
 date_mode_type values that happens to be 0 and misinterpret it as
 the program wanted to pass a NULL pointer to a structure, but that
 is not what happened.

Ah, I didn't think the compiler would coerce an enum into a pointer
constant. That seems kind of insane. But it is indeed what gcc does.

In that case, we can indeed do the NULL-pointer thing I mentioned. Which
is not even _that_ ugly; it follows the standard.

The cast DATE_RELATIVE to a pointer and uncast it on the other side
thing _does_ violate the standard. It is not needed for this, but it
would make the DATE_MODE() macro reentrant.

  +   static const struct fallback_mode = { DATE_NORMAL };
 
 Yes, that is nasty.  Renumbering the enum to begin with 1 may be a
 much saner solution, unless somebody does

I am worried more about somebody who does memset(0) on a struct, and
expects that to default to DATE_NORMAL.

 In any case, I did another evil merge to fix it.

OK. Do you want to leave it be, then, or would you prefer me to do the
NULL fallback? Or we could bump the enum to start with 1, and then
explicitly treat 0 as a synonym for DATE_NORMAL (in case it comes in
through a memset or similar).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[Junio C Hamano]

Jeff King p...@peff.net writes:

 My assumption was that using the raw 0 is something we would frowned
 upon in new code. There was a single historical instance that I fixed in
 the series, but I wouldn't expect new ones (and actually, that instance
 was 1, which would be caught by the compiler).

That is not the problem.

The code on the side branch may add a new callsite, something like
this:

show_ident_date(ident_split, DATE_NORMAL);

based on the current codebase (e.g. 'master' as of today).

The merge goes cleanly, it compiles, even though the new function
signature of show_ident_date(), similar to the updated show_date(),
takes a pointer to a struct where they used to take DATE_$format
constants.

And that is because DATE_NORMAL is defined to be 0; we can claim
that the compiler is being stupid to take one of the enum
date_mode_type values that happens to be 0 and misinterpret it as
the program wanted to pass a NULL pointer to a structure, but that
is not what happened.

 However, if you're concerned, I think we could have show_date massage a
 NULL date, like:

 diff --git a/date.c b/date.c
 index 8f91569..a04d089 100644
 --- a/date.c
 +++ b/date.c
 @@ -173,6 +173,10 @@ const char *show_date(unsigned long time, int tz, const 
 struct date_mode *mode)
  {
   struct tm *tm;
   static struct strbuf timebuf = STRBUF_INIT;
 + static const struct fallback_mode = { DATE_NORMAL };

Yes, that is nasty.  Renumbering the enum to begin with 1 may be a
much saner solution, unless somebody does

if (!mode-type)
/* we know DATE_NORMAL is zero, he he */
do the normal thing;

In any case, I did another evil merge to fix it.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[Junio C Hamano]

Jeff King p...@peff.net writes:

 OK. Do you want to leave it be, then, or would you prefer me to do the
 NULL fallback? Or we could bump the enum to start with 1, and then
 explicitly treat 0 as a synonym for DATE_NORMAL (in case it comes in
 through a memset or similar).

I didn't think about the memset() initialization, and keeping the
normal case to be 0 makes tons of sense.

I'd prefer to see the stale code dump core rather than leaving it
stale without anybody noticing.  Hopefully the date-mode change can
hit 'master' fairly soon after the upcoming release, so unless a fix
that involves show_date() need to be written and applied to 2.4
codebase and upwards at the same time, I do not think it is a huge
issue.

Thanks.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[John Keeping]

On Thu, Jun 25, 2015 at 12:55:02PM -0400, Jeff King wrote:
 In preparation for adding date modes that may carry extra
 information beyond the mode itself, this patch converts the
 date_mode enum into a struct.
 
 Most of the conversion is fairly straightforward; we pass
 the struct as a pointer and dereference the type field where
 necessary. Locations that declare a date_mode can use a {}
 constructor.  However, the tricky case is where we use the
 enum labels as constants, like:
 
   show_date(t, tz, DATE_NORMAL);
 
 Ideally we could say:
 
   show_date(t, tz, { DATE_NORMAL });
 
 but of course C does not allow that.

Yes it does, e.g. in 6.5.2.5 of C11, example 3 shows:

drawline((struct point){.x=1, .y=1},
(struct point){.x=3, .y=4});

The cast is required, but if the argument is pointer-to-const you can
construct a temporary in the function call.

Of course, whether all of the compilers we target support it is a
different question.  If they do, perhaps something like:

#define SIMPLE_DATE(f)  (struct date_mode) { DATE_NORMAL }

would allow the callers to remain reasonably sane.

  Likewise, we cannot
 cast the constant to a struct, because we need to pass an
 actual address. Our options are basically:
 
   1. Manually add a struct date_mode d = { DATE_NORMAL }
  definition to each caller, and pass d. This makes
  the callers uglier, because they sometimes do not even
  have their own scope (e.g., they are inside a switch
  statement).
 
   2. Provide a pre-made global date_normal struct that can
  be passed by address. We'd also need date_rfc2822,
  date_iso8601, and so forth. But at least the ugliness
  is defined in one place.
 
   3. Provide a wrapper that generates the correct struct on
  the fly. The big downside is that we end up pointing to
  a single global, which makes our wrapper non-reentrant.
  But show_date is already not reentrant, so it does not
  matter.
 
 This patch implements 3, along with a minor macro to keep
 the size of the callers sane.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] convert enum date_mode into a struct

[Jeff King]

On Thu, Jun 25, 2015 at 06:03:28PM +0100, John Keeping wrote:

  Ideally we could say:
  
show_date(t, tz, { DATE_NORMAL });
  
  but of course C does not allow that.
 
 Yes it does, e.g. in 6.5.2.5 of C11, example 3 shows:

Well, yes. But we generally restrict ourselves to C89 here, so we are
not even close.

 Of course, whether all of the compilers we target support it is a
 different question.  If they do, perhaps something like:
 
 #define SIMPLE_DATE(f)(struct date_mode) { DATE_NORMAL }
 
 would allow the callers to remain reasonably sane.

My patch already introduces DATE_MODE, so you could conditionally hide
it there, and fall back to date_mode_from_type when the compiler is too
old for this. But then, what is the advantage over the existing
solution? It's reentrant, but I don't think that is a problem here.

And in patch 3, you'll see that I add an extra assertion to
date_mode_from_type that this cannot support (to make sure that we do
not create a DATE_STRFTIME mode with no matching format string). The
syntax above would at least give us NULL for the string, which is better
than random garbage, but I think the assert is better still.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html