Thread: Catching missing Datum conversions
Hi, When reviewing a recent patch, I missed a place where Datum was being converted to another type implicitly (ie without going though a DatumGetXXX() macro). Thanks to Jeff for fixing that (commit b538c90b), but I was curious to see if I could convince my compiler to tell me about that sort of thing. Here's an experimental hack that makes Datum a struct (unfortunately defined in two places, but like I said it's a hack), and then fixes all the resulting compile errors. The main categories of change are: 1. Many cases of replacing "(Datum) 0" with a new macro "NullDatum" and adjusting code that compares with 0/NULL, so you can pretty much ignore that noise. Likewise code that compares datums directly without apparent knowledge of the expected type. 2. VARDATA etc macros taking a Datum instead of a varlena *. I think the interface is suppose to be able to take both, so I think you can pretty much ignore that noise too, I just couldn't immediately think of a trick that would make that polymorphism work so I added DatumGetPointer(x) wherever a Datum x was given directly to those macros. 3. Many cases of object IDs being converted implicitly, for example in syscache calls. A few cases of implicit use as booleans. 4. Various confusions about the types involved in PG_RETURN_XXX and PG_GETARGS_XXX macros, and failure to convert values from Datum-returning functions, or unnecessary conversion of results (eg makeArrayResult). I should probably split this into "actionable" (categories 3 and 4) and "noise and scaffolding" patches. -- Thomas Munro https://enterprisedb.com
Attachment
I should probably split this into "actionable" (categories 3 and 4)
and "noise and scaffolding" patches.
Breaking down the noise-and-scaffolding into some subgroups might make the rather long patches more palatable/exceedingly-obvious:
* (Datum) 0 ---> NullDatum
* 0 ----> NullDatum
* The DatumGetPointer(allParameterTypes) null tests
* The DatumGetPointer(allParameterTypes) null tests
Having said that, everything you did seems really straightforward, except for
src/backend/rewrite/rewriteDefine.csrc/backend/statistics/mcv.csrc/backend/tsearch/ts_parse.c
and those seem like cases where the DatumGetXXX was a no-op before Datum was a struct.
On Fri, Jul 19, 2019 at 7:55 PM Thomas Munro <thomas.munro@gmail.com> wrote: > When reviewing a recent patch, I missed a place where Datum was being > converted to another type implicitly (ie without going though a > DatumGetXXX() macro). Thanks to Jeff for fixing that (commit > b538c90b), but I was curious to see if I could convince my compiler to > tell me about that sort of thing. This is a very easy mistake to make, so if you ever feel like returning to this topic in earnest, I think it could be a worthwhile expenditure of time. -- Robert Haas EDB: http://www.enterprisedb.com