On 10/02/11 22:26, Steve Singer wrote:
> On 11-02-10 03:13 PM, Jan Urbański wrote:
>> On 10/02/11 20:24, Peter Eisentraut wrote:
>
> Here is the rest of my review.
Thanks!
> Ideally char * members of ExceptionMap would be const, but since many
> versions of python take a non-const value to PyErr_NewException that
> won't work :(
Yeah, I got "discards qualifier" warnings when I tried to declare it as
const :(
> After you search the for an exception in the hash you have:
>
> /* We really should find it, but just in case have a fallback */
> Assert(entry != NULL);
> exc = entry ? entry->exc : PLy_exc_spi_error;
>
> I'm not sure the assert is needed here. Just falling back to the
> exception type seems reasonable and more desirable than an assert if
> showhow a new exception gets missed from the list. I don't feel that
> strongly on this.
Maybe the comment doesn't explain this enough. My intention was that in
regular builds you have a fallback and if you're missing an entry in the
exceptions hash, you just get SPIError. But in assert-enabled builds you
get an error, so you can detect it and fix the exceptions hash.
> line 3575: PLy_elog(ERROR, "Failed to add the spiexceptions module");
> "Failed" should be "failed"
Gah, I'll never learn. Will fix.
> Other than that the patch looks fine to me.
Thanks, I'll have the docs ready by today (and I should've have them by
yesterday :/).
Jan