Re: Change definitions of bitmap flags to bit-shifting style

[Andrew Dunstan]

On 12/6/20 11:44 AM, James Coleman wrote:
> On Sun, Dec 6, 2020 at 1:25 AM Michael Paquier  > wrote:
>
> On Sat, Dec 05, 2020 at 10:31:09PM -0300, Alvaro Herrera wrote:
> > The hexadecimal representation is more natural to me than
> bit-shifting,
> > so I would prefer to use that style too.  But maybe I'm trained
> to it
> > because of looking at t_infomask symbols constantly.
>
> If we are going to change all that, hexa style sounds good to me too.
> Would it be worth an addition to the docs, say in [1] to tell that
> this is a preferred style?
>
> [1]: https://www.postgresql.org/docs/devel/source-conventions.html
> ?
> --
> Michael
>
>
>
> In my view the bit shifting approach makes it more obvious a single
> bit is being set, but on the other hand the hex approach makes it
> easier to compare in debugging. 
>
> I’m not really sure which to prefer, though I think I would have
> leaned slightly towards the former. 
>
>

Perhaps we should put one style or the other in a comment. I take Tom's
point, but after the number of bits shifted gets above some number I
have trouble remembering which bit it is, and while of course I can work
it out, it can be a very minor nuisance.


cheers


andrew

Re: Change definitions of bitmap flags to bit-shifting style

[James Coleman]

On Sun, Dec 6, 2020 at 1:25 AM Michael Paquier  wrote:

> On Sat, Dec 05, 2020 at 10:31:09PM -0300, Alvaro Herrera wrote:
> > The hexadecimal representation is more natural to me than bit-shifting,
> > so I would prefer to use that style too.  But maybe I'm trained to it
> > because of looking at t_infomask symbols constantly.
>
> If we are going to change all that, hexa style sounds good to me too.
> Would it be worth an addition to the docs, say in [1] to tell that
> this is a preferred style?
>
> [1]: https://www.postgresql.org/docs/devel/source-conventions.html?
> --
> Michael



In my view the bit shifting approach makes it more obvious a single bit is
being set, but on the other hand the hex approach makes it easier to
compare in debugging.

I’m not really sure which to prefer, though I think I would have leaned
slightly towards the former.

James

>

Re: Change definitions of bitmap flags to bit-shifting style

[Michael Paquier]

On Sat, Dec 05, 2020 at 10:31:09PM -0300, Alvaro Herrera wrote:
> The hexadecimal representation is more natural to me than bit-shifting,
> so I would prefer to use that style too.  But maybe I'm trained to it
> because of looking at t_infomask symbols constantly.

If we are going to change all that, hexa style sounds good to me too.
Would it be worth an addition to the docs, say in [1] to tell that
this is a preferred style? 

[1]: https://www.postgresql.org/docs/devel/source-conventions.html?
--
Michael


signature.asc
Description: PGP signature

Re: Change definitions of bitmap flags to bit-shifting style

[Laurenz Albe]

On Sat, 2020-12-05 at 13:03 -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
> 
> > The attached patch changes definitions like
> >   #define FOO 0x01
> >   #define BAR 0x02
> > to
> >   #define FOO (1 << 0)
> >   #define BAR (1 << 1)
> > etc.
> 
> > Both styles are currently in use, but the latter style seems more 
> > readable and easier to update.
> 
> FWIW, personally I'd vote for doing the exact opposite.  When you are
> debugging and examining the contents of a bitmask variable, it's easier to
> correlate a value like "0x03" with definitions made in the former style.
> Or at least I think so; maybe others see it differently.

+1

Laurenz Albe

Re: Change definitions of bitmap flags to bit-shifting style

[Alvaro Herrera]

On 2020-Dec-05, Tom Lane wrote:

> FWIW, personally I'd vote for doing the exact opposite.  When you are
> debugging and examining the contents of a bitmask variable, it's easier to
> correlate a value like "0x03" with definitions made in the former style.
> Or at least I think so; maybe others see it differently.

The hexadecimal representation is more natural to me than bit-shifting,
so I would prefer to use that style too.  But maybe I'm trained to it
because of looking at t_infomask symbols constantly.

Re: Change definitions of bitmap flags to bit-shifting style

[Tom Lane]

Peter Eisentraut  writes:
> The attached patch changes definitions like
>  #define FOO 0x01
>  #define BAR 0x02
> to
>  #define FOO (1 << 0)
>  #define BAR (1 << 1)
> etc.

> Both styles are currently in use, but the latter style seems more 
> readable and easier to update.

FWIW, personally I'd vote for doing the exact opposite.  When you are
debugging and examining the contents of a bitmask variable, it's easier to
correlate a value like "0x03" with definitions made in the former style.
Or at least I think so; maybe others see it differently.

regards, tom lane

Change definitions of bitmap flags to bit-shifting style

[Peter Eisentraut]

The attached patch changes definitions like

#define FOO 0x01
#define BAR 0x02

to

#define FOO (1 << 0)
#define BAR (1 << 1)

etc.

Both styles are currently in use, but the latter style seems more 
readable and easier to update.


This change only addresses bitmaps used in memory (e.g., for parsing or 
specific function APIs), where the actual bits don't really matter. 
Bits that might go on disk weren't touched.  There, defining the bits in 
a more concrete way seems better.
From 21f7512bb37068e6a8f72d87d4cf31602ce78d9a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 5 Dec 2020 11:45:04 +0100
Subject: [PATCH] Change definitions of bitmap flags to bit-shifting style

Change definitions like

#define FOO 0x01
#define BAR 0x02

to

#define FOO (1 << 0)
#define BAR (1 << 1)

etc.

Both styles are currently in use, but the latter style seems more
readable and easier to update.

This change only addresses bitmaps used in memory (e.g., for parsing
or specific function APIs), where the actual bits don't really
matter.  Bits that might go on disk weren't touched.  There,
defining the bits in a more concrete way seems better.
---
 src/backend/commands/tablecmds.c  | 14 +++---
 src/backend/optimizer/plan/createplan.c   |  8 ++--
 src/backend/optimizer/util/clauses.c  |  2 +-
 src/backend/parser/gram.y | 12 ++---
 src/backend/postmaster/postmaster.c   | 10 ++---
 src/backend/regex/regc_pg_locale.c| 16 +++
 src/backend/storage/buffer/bufmgr.c   |  4 +-
 src/backend/utils/adt/formatting.c| 16 +++
 src/backend/utils/adt/jsonfuncs.c | 10 ++---
 src/backend/utils/adt/ruleutils.c |  6 +--
 src/backend/utils/adt/tsvector_op.c   |  6 +--
 src/backend/utils/adt/varlena.c   |  2 +-
 src/backend/utils/cache/typcache.c| 42 +-
 src/bin/pg_dump/pg_backup_archiver.h  |  6 +--
 src/include/access/nbtree.h   |  4 +-
 src/include/access/skey.h | 18 
 src/include/access/toast_helper.h | 12 ++---
 src/include/catalog/dependency.h  | 14 +++---
 src/include/catalog/heap.h|  6 +--
 src/include/catalog/index.h   | 10 ++---
 src/include/commands/event_trigger.h  |  6 +--
 src/include/common/fe_memutils.h  |  8 ++--
 src/include/executor/executor.h   | 12 ++---
 src/include/foreign/foreign.h |  4 +-
 src/include/miscadmin.h   |  6 +--
 src/include/nodes/extensible.h|  4 +-
 src/include/nodes/nodeFuncs.h | 22 -
 src/include/nodes/nodes.h |  8 ++--
 src/include/nodes/params.h|  2 +-
 src/include/nodes/parsenodes.h| 54 +++
 src/include/nodes/pathnodes.h |  6 +--
 src/include/optimizer/optimizer.h | 16 +++
 src/include/replication/reorderbuffer.h   | 14 +++---
 src/include/storage/dsm.h |  2 +-
 src/include/storage/predicate_internals.h | 24 +-
 src/include/storage/proc.h| 16 +++
 src/include/storage/reinit.h  |  4 +-
 src/include/tcop/utility.h|  6 +--
 src/include/tsearch/ts_public.h   |  6 +--
 src/include/utils/builtins.h  |  8 ++--
 src/include/utils/dsa.h   |  6 +--
 src/include/utils/expandedrecord.h| 16 +++
 src/include/utils/guc_tables.h|  6 +--
 src/include/utils/hsearch.h   | 26 +--
 src/include/utils/lsyscache.h |  4 +-
 src/include/utils/palloc.h|  6 +--
 src/include/utils/regproc.h   |  4 +-
 src/include/utils/sharedtuplestore.h  |  2 +-
 src/include/utils/typcache.h  | 32 +++---
 src/test/isolation/isolationtester.c  |  4 +-
 50 files changed, 276 insertions(+), 276 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 46f1637e77..89fd3bf776 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -289,13 +289,13 @@ struct DropRelationCallbackState
 };
 
 /* Alter table target-type flags for ATSimplePermissions */
-#defineATT_TABLE   0x0001
-#defineATT_VIEW0x0002
-#defineATT_MATVIEW 0x0004
-#defineATT_INDEX   0x0008
-#defineATT_COMPOSITE_TYPE  0x0010
-#defineATT_FOREIGN_TABLE   0x0020
-#defineATT_PARTITIONED_INDEX   0x0040
+#defineATT_TABLE   (1 << 0)
+#defineATT_VIEW(1 << 1)
+#defineATT_MATVIEW (1 <