Обсуждение: Undesirable entries in typedefs list
I noticed that doing pgindent with the current typedefs list available
from the buildfarm caused a lot of havoc in what had been stable code.
Looking into the reasons, it seems that:
(1) "bool" is no longer listed as a typedef name (probably because
stdbool.h makes it a macro instead);
(2) "abs", "boolean", "iterator", "other", "pointer", "reference",
"string", and "type" all now are listed as typedef names.
It's probably okay to treat "boolean" as a typedef, but all those others
are complete disasters. Anyone know where they're coming from?
As for "bool", we could probably deal with that most reliably by
having pgindent add it as a special case. Maybe we could get it
back in there by having some trailing-edge buildfarm member
contribute typedefs, but that seems like a solution with a rather
limited half-life.
regards, tom lane
Hi, On 2018-03-24 13:25:34 -0400, Tom Lane wrote: > (2) "abs", "boolean", "iterator", "other", "pointer", "reference", > "string", and "type" all now are listed as typedef names. > > It's probably okay to treat "boolean" as a typedef, but all those others > are complete disasters. Anyone know where they're coming from? Semi informed theory: LLVM? I think I'd configured one of the LLVM animals to collect typedefs, but that might have been a bad idea... > As for "bool", we could probably deal with that most reliably by > having pgindent add it as a special case. Maybe we could get it > back in there by having some trailing-edge buildfarm member > contribute typedefs, but that seems like a solution with a rather > limited half-life. Could we combine the list of typedefs with one manually maintained in-tree? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-03-24 13:25:34 -0400, Tom Lane wrote:
>> (2) "abs", "boolean", "iterator", "other", "pointer", "reference",
>> "string", and "type" all now are listed as typedef names.
>>
>> It's probably okay to treat "boolean" as a typedef, but all those others
>> are complete disasters. Anyone know where they're coming from?
> Semi informed theory: LLVM? I think I'd configured one of the LLVM
> animals to collect typedefs, but that might have been a bad idea...
That was my first thought as well, since this seems to have changed
quite recently. I don't think it's a bad idea to be collecting typedefs
from something that compiles that code, because otherwise our own
typedefs in that area won't be known.
>> As for "bool", we could probably deal with that most reliably by
>> having pgindent add it as a special case. Maybe we could get it
>> back in there by having some trailing-edge buildfarm member
>> contribute typedefs, but that seems like a solution with a rather
>> limited half-life.
> Could we combine the list of typedefs with one manually maintained
> in-tree?
Perhaps. We might need a manually maintained blacklist, as well.
regards, tom lane
Hi, On 2018-03-24 13:34:45 -0400, Tom Lane wrote: > That was my first thought as well, since this seems to have changed > quite recently. I don't think it's a bad idea to be collecting typedefs > from something that compiles that code, because otherwise our own > typedefs in that area won't be known. The number of typedefs we actually use is fairly small (~5-7), so we could realistically maintain the them manually. I'm about to head out, but afterwards I'm going to check what the typedefs collected actually are when LLVM is enabled. Greetings, Andres Freund
On 2018-03-24 10:41:40 -0700, Andres Freund wrote:
> I'm about to head out, but afterwards I'm going to check what the
> typedefs collected actually are when LLVM is enabled.
It's indeed from llvm:
<3><5201>: Abbrev Number: 4 (DW_TAG_typedef)
<5202> DW_AT_name : (indirect string, offset: 0x8a0a4): string
<5206> DW_AT_decl_file : 13
<5207> DW_AT_decl_line : 74
<5208> DW_AT_type : <0x3896>
which basically references std::string
<3><3896>: Abbrev Number: 24 (DW_TAG_class_type)
<3897> DW_AT_name : (indirect string, offset: 0x2db87): basic_string<char, std::char_traits<char>,
std::allocator<char>>
<389b> DW_AT_byte_size : 32
<389c> DW_AT_decl_file : 12
<389d> DW_AT_decl_line : 77
<389e> DW_AT_sibling : <0x51fc>
Given that the PG code shouldn't refer to this, I think we might be able
to limit things by specifying --dwarf-depth=3 to the objdump output.
I've attached the difference between a objdump typedefs list roughly
equivalent to what the buildfarm uses. There's no difference when not
using llvm.
I'm a bit uncomfortable relying --dwarf-depth=3, with 3 being determined
purely experimentally though.
Greetings,
Andres Freund
Вложения
On Sat, Mar 24, 2018 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote: > I've attached the difference between a objdump typedefs list roughly > equivalent to what the buildfarm uses. There's no difference when not > using llvm. > > I'm a bit uncomfortable relying --dwarf-depth=3, with 3 being determined > purely experimentally though. A quick look at the DWARF4 standard [1] suggests that this refers to lexical depth. So, I agree that that doesn't seem like a great idea. [1] http://dwarfstd.org/doc/DWARF4.pdf -- Peter Geoghegan
On 2018-03-24 14:51:32 -0700, Peter Geoghegan wrote: > On Sat, Mar 24, 2018 at 1:36 PM, Andres Freund <andres@anarazel.de> wrote: > > I've attached the difference between a objdump typedefs list roughly > > equivalent to what the buildfarm uses. There's no difference when not > > using llvm. > > > > I'm a bit uncomfortable relying --dwarf-depth=3, with 3 being determined > > purely experimentally though. > > A quick look at the DWARF4 standard [1] suggests that this refers to > lexical depth. So, I agree that that doesn't seem like a great idea. > > [1] http://dwarfstd.org/doc/DWARF4.pdf Are you referring to Section 3.4? That's something different afaict. Or which bit are you thinking of? See e.g. https://sourceware.org/bugzilla/show_bug.cgi?id=22250 " Another addition "depth" starts counting from zero. Zero usually is the first DIE (compilation_unit or partial_unit). Specifying --dwarf-depth=0 will only print the Compilation Unit headers, followed by a blank line and no ... markers. Unless --dwarf-depth is given with a non-zero argument, then only a blank line is printed. " My understanding is that it controls through how many levels of nesting in type definitions objdump recurses through. The type of typedefs we really care about are going to be at the global level. Which should make them available at dwarf-depth=2. Then we have a few typedefs which are solely done inside functions, which is why we need dwarf-depth=3. Greetings, Andres Freund
On Sun, Mar 25, 2018 at 3:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I noticed that doing pgindent with the current typedefs list available > from the buildfarm caused a lot of havoc in what had been stable code. > Looking into the reasons, it seems that: > > (1) "bool" is no longer listed as a typedef name (probably because > stdbool.h makes it a macro instead); > > (2) "abs", "boolean", "iterator", "other", "pointer", "reference", > "string", and "type" all now are listed as typedef names. > > It's probably okay to treat "boolean" as a typedef, but all those others > are complete disasters. Anyone know where they're coming from? > > As for "bool", we could probably deal with that most reliably by > having pgindent add it as a special case. Maybe we could get it > back in there by having some trailing-edge buildfarm member > contribute typedefs, but that seems like a solution with a rather > limited half-life. > pgindent already has a list of blacklisted typedefs (see lines 121 to 123) cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Mar 25, 2018 at 11:32 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > On Sun, Mar 25, 2018 at 3:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I noticed that doing pgindent with the current typedefs list available >> from the buildfarm caused a lot of havoc in what had been stable code. >> Looking into the reasons, it seems that: >> >> (1) "bool" is no longer listed as a typedef name (probably because >> stdbool.h makes it a macro instead); >> >> (2) "abs", "boolean", "iterator", "other", "pointer", "reference", >> "string", and "type" all now are listed as typedef names. >> >> It's probably okay to treat "boolean" as a typedef, but all those others >> are complete disasters. Anyone know where they're coming from? >> >> As for "bool", we could probably deal with that most reliably by >> having pgindent add it as a special case. Maybe we could get it >> back in there by having some trailing-edge buildfarm member >> contribute typedefs, but that seems like a solution with a rather >> limited half-life. >> > > > pgindent already has a list of blacklisted typedefs (see lines 121 to 123) > Here's a patch to pgindent which I think will do what you want fairly simply, assuming you have identified all the offending tokens. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> pgindent already has a list of blacklisted typedefs (see lines 121 to 123)
Oh, so it does.
> Here's a patch to pgindent which I think will do what you want fairly
> simply, assuming you have identified all the offending tokens.
Hm, this does not work for me (with perl 5.10.1), I get syntax errors like
Not enough arguments for map at src/tools/pgindent/pgindent line 64, near "abs allocfunc iterator other pointer
printfuncreference string type )"
(Might be a runaway multi-line () string starting on line 63)
syntax error at src/tools/pgindent/pgindent line 64, near "abs allocfunc iterator other pointer printfunc reference
stringtype )"
Execution of src/tools/pgindent/pgindent aborted due to compilation errors.
After reading the perlfunc man page I changed
-my %blacklist = map { "$_\n" => 1 }
+my %blacklist = map { +"$_\n" => 1 }
and then it seems to work, but man this is an ugly language :-(
regards, tom lane
On Wed, Mar 28, 2018 at 8:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>>> pgindent already has a list of blacklisted typedefs (see lines 121 to 123)
>
> Oh, so it does.
>
>> Here's a patch to pgindent which I think will do what you want fairly
>> simply, assuming you have identified all the offending tokens.
>
> Hm, this does not work for me (with perl 5.10.1), I get syntax errors like
>
> Not enough arguments for map at src/tools/pgindent/pgindent line 64, near "abs allocfunc iterator other pointer
printfuncreference string type )"
> (Might be a runaway multi-line () string starting on line 63)
> syntax error at src/tools/pgindent/pgindent line 64, near "abs allocfunc iterator other pointer printfunc reference
stringtype )"
> Execution of src/tools/pgindent/pgindent aborted due to compilation errors.
>
> After reading the perlfunc man page I changed
>
> -my %blacklist = map { "$_\n" => 1 }
> +my %blacklist = map { +"$_\n" => 1 }
>
> and then it seems to work, but man this is an ugly language :-(
>
Yes, there are lots of quirks that can make it fun to deal with.
Thanks for fixing and committing.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services