Re: [HACKERS] pgindent weirdness
On Thu, Jul 10, 2014 at 12:02:50PM -0400, Robert Haas wrote: In contrib/test_shm_mq/test.c, the 9.4 pgindent run (0a7832005792fa6dad171f9cadb8d587fe0dd800) did this: -PG_MODULE_MAGIC; -PG_FUNCTION_INFO_V1(test_shm_mq); +PG_MODULE_MAGIC; PG_FUNCTION_INFO_V1(test_shm_mq); PG_FUNCTION_INFO_V1(test_shm_mq_pipelined); This is obviously not an improvement. Is there some formatting rule that I violated in the original code that lead to this, or what? Uh, ever other case of PG_MODULE_MAGIC had blank lines before/after this define. I went and added that to test_shm_mq/test.c, and adjusted other blank lines to be consistent. I did not modify 9.4 as this cosmetic. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Tom Lane wrote:
Bruce Momjian br...@momjian.us writes:
Tom Lane wrote:
Now having said that, there seems to be a pgindent bug here too, in that
it's throwing a space into
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
struct BulkInsertStateData * bistate)
Whether BulkInsertStateData is flagged as a typedef or not, surely it
ought to understand that struct BulkInsertStateData is a type name.
Uh, I think we have this listed as a known bug at the top of the
pgindent script:
Hm. I guess the third observation is that in the current state of the
code, there's no very good reason to be using struct in
RelationGetBufferForTuple's prototype anyway, since the typedef is
declared right above it. Maybe we should just change that and not
risk fooling with pgindent.
Changed as you suggested. I didn't see any other obvious cases.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
new file mode 100644
index 26db1e3..beecc90
*** a/src/backend/access/heap/hio.c
--- b/src/backend/access/heap/hio.c
*** GetVisibilityMapPins(Relation relation,
*** 213,219
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! struct BulkInsertStateData * bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other)
{
bool use_fsm = !(options HEAP_INSERT_SKIP_FSM);
--- 213,219
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! BulkInsertState bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other)
{
bool use_fsm = !(options HEAP_INSERT_SKIP_FSM);
diff --git a/src/include/access/hio.h b/src/include/access/hio.h
new file mode 100644
index 6eac97e..aefd679
*** a/src/include/access/hio.h
--- b/src/include/access/hio.h
*** extern void RelationPutHeapTuple(Relatio
*** 38,44
HeapTuple tuple);
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! struct BulkInsertStateData * bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other);
#endif /* HIO_H */
--- 38,44
HeapTuple tuple);
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
! BulkInsertState bistate,
Buffer *vmbuffer, Buffer *vmbuffer_other);
#endif /* HIO_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Robert Haas wrote:
pgindent seems to have muffed it when it comes to BulkInsertStateData:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 2849992..72a69e5 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -150,7 +150,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer,
int options,
- struct
BulkInsertStateData *bistate)
+ struct
BulkInsertStateData * bistate)
{
booluse_fsm = !(options HEAP_INSERT_SKIP_FSM);
Buffer buffer = InvalidBuffer;
Not sure what happened here exactly...
BulkInsertStateData is not listed in the typedef list supplied by
Andrew; see src/tools/pgindent/typedefs.list. CC'ing him. This might
be because the typdef is listed in two files:
/*
* state for bulk inserts --- private to heapam.c and hio.c
*
* If current_buf isn't InvalidBuffer, then we are holding an extra pin
* on that buffer.
*
* typedef struct BulkInsertStateData *BulkInsertState is in heapam.h
*/
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 05:48 AM, Bruce Momjian wrote:
Robert Haas wrote:
pgindent seems to have muffed it when it comes to BulkInsertStateData:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 2849992..72a69e5 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -150,7 +150,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer,
int options,
- struct
BulkInsertStateData *bistate)
+ struct
BulkInsertStateData * bistate)
{
booluse_fsm = !(options HEAP_INSERT_SKIP_FSM);
Buffer buffer = InvalidBuffer;
Not sure what happened here exactly...
BulkInsertStateData is not listed in the typedef list supplied by
Andrew; see src/tools/pgindent/typedefs.list. CC'ing him. This might
be because the typdef is listed in two files:
/*
* state for bulk inserts --- private to heapam.c and hio.c
*
* If current_buf isn't InvalidBuffer, then we are holding an extra pin
* on that buffer.
*
* typedef struct BulkInsertStateData *BulkInsertState is in heapam.h
*/
It's tagged as a structure type by objdump, but not as a typedef:
140055: Abbrev Number: 4 (DW_TAG_typedef)
40056 DW_AT_name: (indirect string, offset: 0x6bf6):
BulkInsertState
4005a DW_AT_decl_file : 30
4005b DW_AT_decl_line : 32
4005c DW_AT_type: 0x40060
140060: Abbrev Number: 7 (DW_TAG_pointer_type)
40061 DW_AT_byte_size : 8
40062 DW_AT_type: 0x40066
140066: Abbrev Number: 13 (DW_TAG_structure_type)
40067 DW_AT_name: (indirect string, offset: 0x66bf):
BulkInsertStateData
4006b DW_AT_byte_size : 16
4006c DW_AT_decl_file : 31
4006d DW_AT_decl_line : 30
4006e DW_AT_sibling : 0x4008b
I can pull out those too if you want them in the list, but it would
possibly add a LOT of names to the list.
I did carefully warn you about the need to check the effects of the
changes when I committed the new list.
It looks like quite a few of the deletions come into this category, for
example just looking at the diff here
https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list
I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and
AllocChunkData from among the first few that were deleted and all are in
the same category.
I wondered if this is some sort of optimizer effect, but building with
-O0 doesn't seem to affect it.
Note that the list we're using is a composite of dumps from four
platforms: Linux, FreeBSD, MinGW and Cygwin. What they have in common is
that they are all using gcc, and a fairly modern version of gcc at that,
and fairly modern versions of objdump too.
Attached is a partial list of new candidate symbols if we want to pick
up these extras.
cheers
andrew
addrinfo
aff_struct
AggHashEntryData
AggStatePerAggData
AllocBlockData
AllocChunkData
arc
arcbatch
arcp
ArrayIteratorData
ASIdentifiers_st
ASN1_ENCODING_st
asn1_object_st
asn1_string_st
asn1_type_st
ASN1_VALUE_st
attrDefault
AUTHORITY_KEYID_st
BgWriterSlotMapping
bignum_ctx
bignum_st
bio_method_st
bio_st
bkend
bn_blinding_st
bn_gencb_st
bn_mont_ctx_st
BufferAccessStrategyData
buf_mem_st
buftag
BulkInsertStateData
cachedesc
carc
catcache
catcacheheader
catclist
catctup
cert_st
cname
cnfa
colordesc
colormap
colors
comp_ctx_st
comp_method_st
config_bool
config_enum
config_enum_entry
config_generic
config_int
config_real
config_string
ConnectionOption
constrCheck
crypto_ex_data_st
cvec
_DestReceiver
dfa
df_files
dh_method
dh_st
dirent
__dirstream
dropmsgstrings
dsa_method
DSA_SIG_st
dsa_st
dtls1_bitmap_st
dtls1_retransmit_state
dtls1_state_st
dtls1_timeout_st
encoding_match
engine_st
env_md_ctx_st
env_md_st
evp_cipher_ctx_st
evp_cipher_st
evp_pkey_asn1_method_st
evp_pkey_ctx_st
evp_pkey_st
fmgr_security_definer_cache
fns
fp_info
_FuncCandidateList
GinScanEntryData
GinScanKeyData
GlobalTransactionData
group
gss_buffer_desc_struct
gss_channel_bindings_struct
gss_cred_id_struct
gss_ctx_id_struct
gss_name_struct
gss_OID_desc_struct
guc_stack
guts
HashJoinTableData
HashJoinTupleData
HeapScanDescData
hmac_ctx_st
hm_header_st
ifaddrs
in6_addr
in_addr
_IndexList
IndexScanDescData
_IO_FILE
_IO_marker
ipc_perm
ISSUING_DIST_POINT_st
itimerval
__jmp_buf_tag
kssl_ctx_st
lconv
lc_time_T
ldap
ldapmsg
lhash_st_SSL_SESSION
__locale_data
__locale_struct
lsinfo
mbinterval
_MdfdVec
MergeJoinClauseData
NAME_CONSTRAINTS_st
nameData
nfa
NumericData
NumericLong
NumericShort
OldSerXidControlData
ONEXIT
opclasscacheent
pam_conv
pam_handle
pam_message
pam_response
ParamListInfoData
passwd
pg_encoding
pg_tm
pollfd
PortalData
portalhashent
_pqueue
PredXactListData
PredXactListElementData
ptrs
Re: [HACKERS] pgindent weirdness
Andrew Dunstan and...@dunslane.net writes: On 04/20/2011 05:48 AM, Bruce Momjian wrote: BulkInsertStateData is not listed in the typedef list supplied by Andrew; see src/tools/pgindent/typedefs.list. CC'ing him. This might be because the typdef is listed in two files: It's tagged as a structure type by objdump, but not as a typedef: Hmm. hio.h clearly declares it as both, but many object files probably include only heapam.h, which exposes only the struct name. I'm guessing that you are merging the results from objdump'ing different files in a way that fails to consider the possibility of some files knowing more versions of a symbol than others. Now having said that, there seems to be a pgindent bug here too, in that it's throwing a space into Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, struct BulkInsertStateData * bistate) Whether BulkInsertStateData is flagged as a typedef or not, surely it ought to understand that struct BulkInsertStateData is a type name. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 04/20/2011 05:48 AM, Bruce Momjian wrote: BulkInsertStateData is not listed in the typedef list supplied by Andrew; see src/tools/pgindent/typedefs.list. CC'ing him. This might be because the typdef is listed in two files: It's tagged as a structure type by objdump, but not as a typedef: Hmm. hio.h clearly declares it as both, but many object files probably include only heapam.h, which exposes only the struct name. I'm guessing that you are merging the results from objdump'ing different files in a way that fails to consider the possibility of some files knowing more versions of a symbol than others. Now having said that, there seems to be a pgindent bug here too, in that it's throwing a space into Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, struct BulkInsertStateData * bistate) Whether BulkInsertStateData is flagged as a typedef or not, surely it ought to understand that struct BulkInsertStateData is a type name. Uh, I think we have this listed as a known bug at the top of the pgindent script: # Known bugs: # # Blank line is added after parentheses; seen as a function definition, no space # after *: # y = (int) x *y; # # Structure/union pointers in function prototypes and definitions have an extra # space after the asterisk: # # void x(struct xxc * a); -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Now having said that, there seems to be a pgindent bug here too, in that it's throwing a space into Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, struct BulkInsertStateData * bistate) Whether BulkInsertStateData is flagged as a typedef or not, surely it ought to understand that struct BulkInsertStateData is a type name. Uh, I think we have this listed as a known bug at the top of the pgindent script: Hm. I guess the third observation is that in the current state of the code, there's no very good reason to be using struct in RelationGetBufferForTuple's prototype anyway, since the typedef is declared right above it. Maybe we should just change that and not risk fooling with pgindent. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Tom Lane wrote: Now having said that, there seems to be a pgindent bug here too, in that it's throwing a space into Buffer RelationGetBufferForTuple(Relation relation, Size len, Buffer otherBuffer, int options, struct BulkInsertStateData * bistate) Whether BulkInsertStateData is flagged as a typedef or not, surely it ought to understand that struct BulkInsertStateData is a type name. Uh, I think we have this listed as a known bug at the top of the pgindent script: Hm. I guess the third observation is that in the current state of the code, there's no very good reason to be using struct in RelationGetBufferForTuple's prototype anyway, since the typedef is declared right above it. Maybe we should just change that and not risk fooling with pgindent. Probably. :-( -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 11:43 AM, Tom Lane wrote: Andrew Dunstanand...@dunslane.net writes: On 04/20/2011 05:48 AM, Bruce Momjian wrote: BulkInsertStateData is not listed in the typedef list supplied by Andrew; see src/tools/pgindent/typedefs.list. CC'ing him. This might be because the typdef is listed in two files: It's tagged as a structure type by objdump, but not as a typedef: Hmm. hio.h clearly declares it as both, but many object files probably include only heapam.h, which exposes only the struct name. I'm guessing that you are merging the results from objdump'ing different files in a way that fails to consider the possibility of some files knowing more versions of a symbol than others. We don't run objdump against individual object files, we run it against the linked binaries, i.e. the contents of the installed bin and lib directories. But in any case, *none* of the individual files knows about BulkInsertStateData as a typedef: [andrew@emma backend]$ for f in ./access/heap/hio.o ./access/heap/heapam.o ./commands/tablecmds.o ./commands/copy.o ./executor/execMain.o ; do objdump -W $f ; done | egrep -A3 DW_TAG_typedef | grep BulkInsertState 1811 DW_AT_name: (indirect string, offset: 0x1cc9): BulkInsertState 1f3c DW_AT_name: (indirect string, offset: 0x296c): BulkInsertState 1fac DW_AT_name: (indirect string, offset: 0x5c5b): BulkInsertState 211b DW_AT_name: (indirect string, offset: 0x35ad): BulkInsertState 2530 DW_AT_name: (indirect string, offset: 0x3c93): BulkInsertState And the reason is actually fairly obvious on closer inspection. The only place we actually use the BulkInsertStateData typedef (as opposed to the struct declaration) is here: ./backend/access/heap/heapam.c:bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData)); and that sizeof operation will be resolved at compile time and never hit the symbol table. So I suspect that the typedef finding code is actually working just fine. It looks like the real problem is in how pgindent handles the use of 'struct foo' in various places. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Andrew Dunstan and...@dunslane.net writes: But in any case, *none* of the individual files knows about BulkInsertStateData as a typedef: ... And the reason is actually fairly obvious on closer inspection. The only place we actually use the BulkInsertStateData typedef (as opposed to the struct declaration) is here: ./backend/access/heap/heapam.c:bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData)); and that sizeof operation will be resolved at compile time and never hit the symbol table. Oh, interesting. So you're saying that for this mechanism to know that foo is a typedef, there has to be at least one variable in the code that's declared as being of type foo or foo *? (Where variable would include function parameters, fields of other structs, etc.) That's probably fine, because otherwise we'd have the typedef list cluttered with junk we don't care about from system headers. So in the case at hand, we actually *need* to remove the struct from RelationGetBufferForTuple's declaration, so that BulkInsertStateData gets used as a typedef name in that way. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 12:29 PM, Tom Lane wrote: Andrew Dunstanand...@dunslane.net writes: But in any case, *none* of the individual files knows about BulkInsertStateData as a typedef: ... And the reason is actually fairly obvious on closer inspection. The only place we actually use the BulkInsertStateData typedef (as opposed to the struct declaration) is here: ./backend/access/heap/heapam.c:bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData)); and that sizeof operation will be resolved at compile time and never hit the symbol table. Oh, interesting. So you're saying that for this mechanism to know that foo is a typedef, there has to be at least one variable in the code that's declared as being of type foo or foo *? (Where variable would include function parameters, fields of other structs, etc.) I believe so. I don't see how it could get tagged in the tables otherwise. That's probably fine, because otherwise we'd have the typedef list cluttered with junk we don't care about from system headers. Well, yes, except that I'm a tiny bit smarter than that :-) After we generate the list of symbols we check that they actually occur in our sources and filter them out if they don't. That reduces the list by quite a lot. So in the case at hand, we actually *need* to remove the struct from RelationGetBufferForTuple's declaration, so that BulkInsertStateData gets used as a typedef name in that way. That sounds right. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On Wed, Apr 20, 2011 at 12:38 PM, Andrew Dunstan and...@dunslane.net wrote:
So in the case at hand, we actually *need* to remove the struct from
RelationGetBufferForTuple's declaration, so that BulkInsertStateData
gets used as a typedef name in that way.
Since the general form seems to be to declare things as:
typedef struct foo { ... } foo;
Is there any reason why we see any struct foo in the sources other
than in the typedef line?
Legacy and invasive patch are good enough reasons, if they are it...
a.
--
Aidan Van Dyk Create like a god,
ai...@highrise.ca command like a king,
http://www.highrise.ca/ work like a slave.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Aidan Van Dyk ai...@highrise.ca writes:
Since the general form seems to be to declare things as:
typedef struct foo { ... } foo;
Is there any reason why we see any struct foo in the sources other
than in the typedef line?
It gives an escape hatch in case you need a forward reference to the
struct, ie you can do struct foo * even before this. But I agree that
90% of those struct tags are useless, and so the habit of tagging every
typedef this way is mostly legacy.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan and...@dunslane.net wrote: I did carefully warn you about the need to check the effects of the changes when I committed the new list. It looks like quite a few of the deletions come into this category, for example just looking at the diff here https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and AllocChunkData from among the first few that were deleted and all are in the same category. This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Robert Haas wrote: On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstan and...@dunslane.net wrote: I did carefully warn you about the need to check the effects of the changes when I committed the new list. It looks like quite a few of the deletions come into this category, for example just looking at the diff here https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and AllocChunkData from among the first few that were deleted and all are in the same category. This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? I think Andrew also supplied the typedef list for the 9.0 run. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 01:12 PM, Robert Haas wrote: On Wed, Apr 20, 2011 at 11:15 AM, Andrew Dunstanand...@dunslane.net wrote: I did carefully warn you about the need to check the effects of the changes when I committed the new list. It looks like quite a few of the deletions come into this category, for example just looking at the diff here https://github.com/postgres/postgres/commit/fe1438da8aa8a45f2cee816eb54841f97d3b2f22#src/tools/pgindent/typedefs.list I see AggHashEntryData, AggStatePerAggData, AllocBlockData, and AllocChunkData from among the first few that were deleted and all are in the same category. This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? We have newer compilers which probably construct the symbol tables slightly differently. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 01:16 PM, Bruce Momjian wrote: This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? I think Andrew also supplied the typedef list for the 9.0 run. Yes. But in November, the server where all my animals were running died. The rebuilt machines all used newer versions of the OS, new compilers and newer tools such as objdump. As I pointed out at the time I committed the new typedefs list, that accounts for a lot of the changes. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 01:10 PM, Tom Lane wrote:
Aidan Van Dykai...@highrise.ca writes:
Since the general form seems to be to declare things as:
typedef struct foo { ... } foo;
Is there any reason why we see any struct foo in the sources other
than in the typedef line?
It gives an escape hatch in case you need a forward reference to the
struct, ie you can do struct foo * even before this. But I agree that
90% of those struct tags are useless, and so the habit of tagging every
typedef this way is mostly legacy.
Yeah, I think it would be reasonable to remove lots of them, especially
in argument lists where I think they're a bit ugly anyway.
I'm not sure if now is a good time to be doing that sort of cleanup -
maybe we should just add the typedefs we think we're missing to the
typedefs list and try another pgindent run, and then make these changes
early in 9.2 dev cycle.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Andrew Dunstan and...@dunslane.net writes: On 04/20/2011 01:16 PM, Bruce Momjian wrote: This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? I think Andrew also supplied the typedef list for the 9.0 run. Yes. But in November, the server where all my animals were running died. The rebuilt machines all used newer versions of the OS, new compilers and newer tools such as objdump. As I pointed out at the time I committed the new typedefs list, that accounts for a lot of the changes. It wouldn't surprise me in the least to find out that newer gcc's stopped emitting symbol table entries for unreferenced typedefs. In fact, using HEAD, I get this on my old HPUX box: (gdb) p sizeof(BulkInsertStateData) $65 = 8 and this on my Fedora 13 box: (gdb) p sizeof(BulkInsertStateData) No symbol BulkInsertStateData in current context. (gcc 2.95.3 and 4.4.5 respectively) So the tools definitely changed sometime in the last N years. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 04/20/2011 01:16 PM, Bruce Momjian wrote: This implies to me that we changed something about how we handle this since we did the 9.0 runs, but I don't know what it was. Should I? I think Andrew also supplied the typedef list for the 9.0 run. Yes. But in November, the server where all my animals were running died. The rebuilt machines all used newer versions of the OS, new compilers and newer tools such as objdump. As I pointed out at the time I committed the new typedefs list, that accounts for a lot of the changes. It wouldn't surprise me in the least to find out that newer gcc's stopped emitting symbol table entries for unreferenced typedefs. In fact, using HEAD, I get this on my old HPUX box: (gdb) p sizeof(BulkInsertStateData) $65 = 8 and this on my Fedora 13 box: (gdb) p sizeof(BulkInsertStateData) No symbol BulkInsertStateData in current context. (gcc 2.95.3 and 4.4.5 respectively) So the tools definitely changed sometime in the last N years. So the list of possible additions Andrew supplied are cases where we never reference those typedefs --- seems like a cleanup opportunity. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 04:28 PM, Bruce Momjian wrote:
So the list of possible additions Andrew supplied are cases where we
never reference those typedefs --- seems like a cleanup opportunity.
I think the best cleanup idea is Aidan's, namely is we have declared
typdef struct foo { ... } foo; we should use foo in the code
instead of struct foo. Then the typedef will be referenced, and the
code will be cleaner, and we won't run into the pgindent struct bug
either, so it's a win/win/win.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Andrew Dunstan and...@dunslane.net writes:
On 04/20/2011 04:28 PM, Bruce Momjian wrote:
So the list of possible additions Andrew supplied are cases where we
never reference those typedefs --- seems like a cleanup opportunity.
I think the best cleanup idea is Aidan's, namely is we have declared
typdef struct foo { ... } foo; we should use foo in the code
instead of struct foo. Then the typedef will be referenced, and the
code will be cleaner, and we won't run into the pgindent struct bug
either, so it's a win/win/win.
We want to do that in any case. I think that Bruce was suggesting going
further and actively removing unreferenced struct tags from the
declaration sites. I'm less enthused about that. It would save nothing
except some probably-unmeasurable amount of compile time, and it'd
result in a lot of diffs that might come back to bite future
back-patching efforts.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
Tom Lane wrote:
Andrew Dunstan and...@dunslane.net writes:
On 04/20/2011 04:28 PM, Bruce Momjian wrote:
So the list of possible additions Andrew supplied are cases where we
never reference those typedefs --- seems like a cleanup opportunity.
I think the best cleanup idea is Aidan's, namely is we have declared
typdef struct foo { ... } foo; we should use foo in the code
instead of struct foo. Then the typedef will be referenced, and the
code will be cleaner, and we won't run into the pgindent struct bug
either, so it's a win/win/win.
We want to do that in any case. I think that Bruce was suggesting going
further and actively removing unreferenced struct tags from the
declaration sites. I'm less enthused about that. It would save nothing
except some probably-unmeasurable amount of compile time, and it'd
result in a lot of diffs that might come back to bite future
back-patching efforts.
No, I wasn't thinking that far; just finding the cases where we don't
reference a typedef and instead use 'struct structname'. I think Andrew
has supplied that list, almost.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent weirdness
On 04/20/2011 05:29 PM, Tom Lane wrote:
Andrew Dunstanand...@dunslane.net writes:
On 04/20/2011 04:28 PM, Bruce Momjian wrote:
So the list of possible additions Andrew supplied are cases where we
never reference those typedefs --- seems like a cleanup opportunity.
I think the best cleanup idea is Aidan's, namely is we have declared
typdef struct foo { ... } foo; we should use foo in the code
instead of struct foo. Then the typedef will be referenced, and the
code will be cleaner, and we won't run into the pgindent struct bug
either, so it's a win/win/win.
We want to do that in any case. I think that Bruce was suggesting going
further and actively removing unreferenced struct tags from the
declaration sites. I'm less enthused about that. It would save nothing
except some probably-unmeasurable amount of compile time, and it'd
result in a lot of diffs that might come back to bite future
back-patching efforts.
Well he says not, but in any case I agree there's no great gain from it.
It's a well established C idiom, and as you pointed out upthread the
struct tag is just about required for defining recursive structs anyway.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
