| 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
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?
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 )
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.
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.
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;