On Mon, May 16, 2022 at 1:58 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
>
> Inspired by [0], I looked to convert more macros to inline functions.
> The attached patches are organized "bottom up" in terms of their API
> layering; some of the later ones depend on some of the earlier ones.
>
All the patches look good to me, except the following that are minor
things that can be ignored if you want.
0002 patch:
+static inline OffsetNumber
+PageGetMaxOffsetNumber(Page page)
+{
+ if (((PageHeader) page)->pd_lower <= SizeOfPageHeaderData)
+ return 0;
+ else
+ return ((((PageHeader) page)->pd_lower - SizeOfPageHeaderData)
/ sizeof(ItemIdData));
+}
The "else" is not necessary, we can have the return statement directly
which would save some indentation as well. The Similar pattern can be
considered for 0004 and 0007 patches as well.
--
0004 patch:
+static inline void
+XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo
*logSegNo, int wal_segsz_bytes)
+{
+ uint32 log;
+ uint32 seg;
+ sscanf(fname, "%08X%08X%08X", tli, &log, &seg);
+ *logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
+}
Can we have a blank line after variable declarations that we usually have?
--
0006 patch:
+static inline Datum
+fetch_att(const void *T, bool attbyval, int attlen)
+{
+ if (attbyval)
+ {
+#if SIZEOF_DATUM == 8
+ if (attlen == sizeof(Datum))
+ return *((const Datum *) T);
+ else
+#endif
Can we have a switch case like store_att_byval() instead of if-else,
code would look more symmetric, IMO.
Regards,
Amul
