Re: [PATCH 1/4] Microsoft Visual C patches

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

On Thu, Jun 19, 2014 at 05:17:40PM +0300, Andrei Errapart wrote:
> First, thanks for accepting 3 of the patches!
> 
> 19.06.2014 14:14, David Gibson kirjutas:
> >Ugh, this is by far the ugliest of these workarounds, and the reason
> >for it in the code is really non-obvious.  I'll have to think about
> >this some more.
> 
> Good point. Kind of careless of me not to pay attention to that aspect.
> 
> What about dropping the field "num_prereqs" altogether? It forces someone
> looping over the field "prereq" to think about the end condition and
> comparision with NULL (or nullptr) is an obvious answer. A diff is at the
> end of the letter.

Hm, that might work.  I'll think on it.

> >>     * Initialize check->inprogress, too.
> >
> >Why..?
> 
> Stumbled over that one - the value of inprogress was in some cases
> initialized to "true" when compiled with MSVC.
> 
> According to the C99 draft
> (http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf, pages 137,138),
> in static storage all fields are initialized to zero, null, 0.

Exactly..

> According to MSDN (http://msdn.microsoft.com/en-us/library/81k8cwsz.aspx),
> initilization is required: "If initializer-list has fewer values than an
> aggregate type, the remaining members or elements of the aggregate type are
> initialized to 0. The initial value of an automatic identifier not
> explicitly initialized is undefined."

Right, an automatic identifier, which is to say a local variable, not
a global variable like this,

> >I've applied patches 2, 3, and 4.  But for future reference, please remember:
> >
> >   * Patches should be inline, not attachments
> >   * Commit message and signed-off-by lines should not be indented
> >   * Commit messages need more details on the reason for the patch (see
> >     the changes I've made for examples).
> >   * Commit messages need a 1 line summary at the top (2, 3, and 4 have
> >     this, but 1 doesn't)
> >   * There should be a blank line between the 1-lite summary and the
> >     remainder of the commit message
> 
> Thanks again.
> 
> Is there an automated command to format the commit message per the
> requirements?

git format-patch should do it, or git send-email will format and send
out the emails for you too.

> diff --git a/checks.c b/checks.c
> index 47eda65..fe90dfc 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -54,12 +54,11 @@ struct check {
>  	bool warn, error;
>  	enum checkstatus status;
>  	bool inprogress;
> -	int num_prereqs;
>  	struct check **prereq;
>  };
> 
>  #define CHECK_ENTRY(nm, tfn, nfn, pfn, d, w, e, ...)	       \
> -	static struct check *nm##_prereqs[] = { __VA_ARGS__ }; \
> +	static struct check *nm##_prereqs[] = { __VA_ARGS__, NULL }; \
>  	static struct check nm = { \
>  		.name = #nm, \
>  		.tree_fn = (tfn), \
> @@ -69,7 +68,6 @@ struct check {
>  		.warn = (w), \
>  		.error = (e), \
>  		.status = UNCHECKED, \
> -		.num_prereqs = ARRAY_SIZE(nm##_prereqs), \
>  		.prereq = nm##_prereqs, \
>  	};
>  #define WARNING(nm, tfn, nfn, pfn, d, ...) \
> @@ -153,7 +151,7 @@ static bool run_check(struct check *c, struct node *dt)
> 
>  	c->inprogress = true;
> 
> -	for (i = 0; i < c->num_prereqs; i++) {
> +	for (i = 0; c->prereq[i] != NULL; i++) {
>  		struct check *prq = c->prereq[i];
>  		error = error || run_check(prq, dt);
>  		if (prq->status != PASSED) {
> @@ -192,7 +190,7 @@ static inline void check_always_fail(struct check *c,
> struct node *dt)
>  {
>  	FAIL(c, "always_fail check");
>  }
> -TREE_CHECK(always_fail, NULL);
> +TREE_CHECK(always_fail, NULL, NULL);

Why is the extra NULL necessary here (and similar places)?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]