Patchwork parsers: fix typing and sign issues

login
register
mail settings
Submitter Henrik Stuart
Date Sept. 6, 2014, 8:25 p.m.
Message ID <41abcba2e5d08ac00abf.1410035137@yggdrasil>
Download mbox | patch
Permalink /patch/5711/
State Changes Requested
Headers show

Comments

Henrik Stuart - Sept. 6, 2014, 8:25 p.m.
# HG changeset patch
# User Henrik Stuart <hg@hstuart.dk>
# Date 1410029932 -7200
#      Sat Sep 06 20:58:52 2014 +0200
# Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
# Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
parsers: fix typing and sign issues

Normalized the use of types in the parser to avoid signed/unsigned mismatches
and normalized const-ness call to free to omit warnings with both gcc and
Microsoft Visual C++.

All the fixes were based on warnings from Microsoft Visual C++ 2008.
Pierre-Yves David - Sept. 8, 2014, 11:59 a.m.
On 09/06/2014 10:25 PM, Henrik Stuart wrote:
> # HG changeset patch
> # User Henrik Stuart <hg@hstuart.dk>
> # Date 1410029932 -7200
> #      Sat Sep 06 20:58:52 2014 +0200
> # Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
> # Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
> parsers: fix typing and sign issues
>
> Normalized the use of types in the parser to avoid signed/unsigned mismatches
> and normalized const-ness call to free to omit warnings with both gcc and
> Microsoft Visual C++.


Look legit, but could we g et different patches for different class of 
fixes?
Matt Mackall - Sept. 9, 2014, 8:58 p.m.
On Sat, 2014-09-06 at 22:25 +0200, Henrik Stuart wrote:
> # HG changeset patch
> # User Henrik Stuart <hg@hstuart.dk>
> # Date 1410029932 -7200
> #      Sat Sep 06 20:58:52 2014 +0200
> # Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
> # Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
> parsers: fix typing and sign issues
> 
> Normalized the use of types in the parser to avoid signed/unsigned mismatches
> and normalized const-ness call to free to omit warnings with both gcc and
> Microsoft Visual C++.

> -		free(self->offsets);
> +		free((char**)self->offsets);

Bleck. I can't get GCC to give a warning about this, can you?

It might just be better to remove the const on self->offsets, as the C
standard really isn't up to the task of handling const-correctness on
doubly-indirect pointers and people like me are inclined to remove
apparently useless casts like this on sight.

(people who are curious about the language lawyer details can start
here: http://c-faq.com/ansi/constmismatch.html )
Henrik Stuart - Sept. 10, 2014, 6:34 a.m.
On 2014-09-09 22:58, Matt Mackall wrote:
> On Sat, 2014-09-06 at 22:25 +0200, Henrik Stuart wrote:
>> # HG changeset patch
>> # User Henrik Stuart <hg@hstuart.dk>
>> # Date 1410029932 -7200
>> #      Sat Sep 06 20:58:52 2014 +0200
>> # Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
>> # Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
>> parsers: fix typing and sign issues
>>
>> Normalized the use of types in the parser to avoid signed/unsigned 
>> mismatches
>> and normalized const-ness call to free to omit warnings with both 
>> gcc and
>> Microsoft Visual C++.
>
>> -		free(self->offsets);
>> +		free((char**)self->offsets);
>
> Bleck. I can't get GCC to give a warning about this, can you?

Not for const char**, but it'll happily warn about it with const char*, 
strangely
enough. I believe gcc is wrong in this regard.

> It might just be better to remove the const on self->offsets, as the 
> C
> standard really isn't up to the task of handling const-correctness on
> doubly-indirect pointers and people like me are inclined to remove
> apparently useless casts like this on sight.

Sounds like a reasonable solution. I'll post a V3 of this fix. The 
remainder of the
V2 series for parsers should still be good to be queued, I reckon.
Steve Borho - Sept. 10, 2014, 8:40 a.m.
On 09/09, Matt Mackall wrote:
> On Sat, 2014-09-06 at 22:25 +0200, Henrik Stuart wrote:
> > # HG changeset patch
> > # User Henrik Stuart <hg@hstuart.dk>
> > # Date 1410029932 -7200
> > #      Sat Sep 06 20:58:52 2014 +0200
> > # Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
> > # Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
> > parsers: fix typing and sign issues
> > 
> > Normalized the use of types in the parser to avoid signed/unsigned mismatches
> > and normalized const-ness call to free to omit warnings with both gcc and
> > Microsoft Visual C++.
> 
> > -		free(self->offsets);
> > +		free((char**)self->offsets);
> 
> Bleck. I can't get GCC to give a warning about this, can you?
> 
> It might just be better to remove the const on self->offsets, as the C
> standard really isn't up to the task of handling const-correctness on
> doubly-indirect pointers and people like me are inclined to remove
> apparently useless casts like this on sight.
> 
> (people who are curious about the language lawyer details can start
> here: http://c-faq.com/ansi/constmismatch.html )

I'm pretty sure the warnings Henrik is fixing are coming from VC9.
Matt Mackall - Sept. 10, 2014, 8:19 p.m.
On Wed, 2014-09-10 at 08:34 +0200, Henrik Stuart wrote:
> On 2014-09-09 22:58, Matt Mackall wrote:
> > On Sat, 2014-09-06 at 22:25 +0200, Henrik Stuart wrote:
> >> # HG changeset patch
> >> # User Henrik Stuart <hg@hstuart.dk>
> >> # Date 1410029932 -7200
> >> #      Sat Sep 06 20:58:52 2014 +0200
> >> # Node ID 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e
> >> # Parent  c5df4af17110838b713d6c9ec3b824fb0b6c1b33
> >> parsers: fix typing and sign issues
> >>
> >> Normalized the use of types in the parser to avoid signed/unsigned 
> >> mismatches
> >> and normalized const-ness call to free to omit warnings with both 
> >> gcc and
> >> Microsoft Visual C++.
> >
> >> -		free(self->offsets);
> >> +		free((char**)self->offsets);
> >
> > Bleck. I can't get GCC to give a warning about this, can you?
> 
> Not for const char**, but it'll happily warn about it with const char*, 
> strangely
> enough. I believe gcc is wrong in this regard.

Well, there's an old philosophical argument about whether the const-ness
of an argument ends when its lifetime ends (ie when you call free). C
definitely falls on the "no" side of the debate, based on the way the
standard specifies const and free[1]. The free() call will itself
probably modify the contents of pointer so it's considered to violate
the const semantics and thus takes an unqualified void * (even though
behavior of pointer accesses after entry to free is definitely in
"undefined" territory).

C++ falls on the "yes" side of the debate: you can both initialize and
destroy const object and const-ness begins when the constructor ends and
ends when the destructor begins.

[1] Though there are things like kfree(const void *) in the Linux kernel
that take a different view.

Patch

diff -r c5df4af17110838b713d6c9ec3b824fb0b6c1b33 -r 41abcba2e5d08ac00abf2df5f6cbe2fe3a7e2e6e mercurial/parsers.c
--- a/mercurial/parsers.c	Thu Sep 04 09:59:23 2014 -0400
+++ b/mercurial/parsers.c	Sat Sep 06 20:58:52 2014 +0200
@@ -275,15 +275,20 @@ 
 	PyObject *fname = NULL, *cname = NULL, *entry = NULL;
 	char state, *cur, *str, *cpos;
 	int mode, size, mtime;
-	unsigned int flen;
-	int len, pos = 40;
+	unsigned int flen, len, pos = 40;
+	int readlen;
 
 	if (!PyArg_ParseTuple(args, "O!O!s#:parse_dirstate",
 			      &PyDict_Type, &dmap,
 			      &PyDict_Type, &cmap,
-			      &str, &len))
+			      &str, &readlen))
 		goto quit;
 
+	if (readlen < 0)
+		goto quit;
+
+	len = readlen;
+
 	/* read parents */
 	if (len < 40)
 		goto quit;
@@ -524,7 +529,7 @@ 
 static PyObject *nullentry;
 static const char nullid[20];
 
-static long inline_scan(indexObject *self, const char **offsets);
+static Py_ssize_t inline_scan(indexObject *self, const char **offsets);
 
 #if LONG_MAX == 0x7fffffffL
 static char *tuple_format = "Kiiiiiis#";
@@ -681,10 +686,10 @@ 
 {
 	PyObject *obj;
 	char *node;
-	long offset;
+	Py_ssize_t offset;
 	Py_ssize_t len, nodelen;
 
-	if (!PyArg_ParseTuple(args, "lO", &offset, &obj))
+	if (!PyArg_ParseTuple(args, "nO", &offset, &obj))
 		return NULL;
 
 	if (!PyTuple_Check(obj) || PyTuple_GET_SIZE(obj) != 8) {
@@ -739,7 +744,7 @@ 
 		self->cache = NULL;
 	}
 	if (self->offsets) {
-		free(self->offsets);
+		free((char**)self->offsets);
 		self->offsets = NULL;
 	}
 	if (self->nt) {
@@ -881,7 +886,7 @@ 
 
 		if (nothead[i])
 			continue;
-		head = PyInt_FromLong(i);
+		head = PyInt_FromSsize_t(i);
 		if (head == NULL || PyList_Append(heads, head) == -1) {
 			Py_XDECREF(head);
 			goto bail;
@@ -1304,7 +1309,7 @@ 
 	PyObject *gca = PyList_New(0);
 	int i, v, interesting;
 	int maxrev = -1;
-	long sp;
+	bitmask sp;
 	bitmask *seen;
 
 	if (gca == NULL)
@@ -1327,7 +1332,7 @@ 
 	interesting = revcount;
 
 	for (v = maxrev; v >= 0 && interesting; v--) {
-		long sv = seen[v];
+		bitmask sv = seen[v];
 		int parents[2];
 
 		if (!sv)
@@ -1853,7 +1858,7 @@ 
  * Find all RevlogNG entries in an index that has inline data. Update
  * the optional "offsets" table with those entries.
  */
-static long inline_scan(indexObject *self, const char **offsets)
+static Py_ssize_t inline_scan(indexObject *self, const char **offsets)
 {
 	const char *data = PyString_AS_STRING(self->data);
 	Py_ssize_t pos = 0;
@@ -1913,7 +1918,7 @@ 
 	Py_INCREF(self->data);
 
 	if (self->inlined) {
-		long len = inline_scan(self, NULL);
+		Py_ssize_t len = inline_scan(self, NULL);
 		if (len == -1)
 			goto bail;
 		self->raw_length = len;