Re: typedefs for indent

bruce wrote:
> Andrew Dunstan wrote:
> > This URL <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl> gives a
> > typedef list that is (currently) the combined result from three fairly
> > different buildfarm members:
> >
> > dungbeetle | 2009-03-22 06:44:01
> > brown_bat | 2009-03-21 13:00:58
> > dawn_bat | 2009-03-21 14:23:40
> >
> > These are respectively my Linux, Cygwin and MinGW buildfarm members.
> >
> > I don't have a BSD machine of any flavor to test on, and I don't know
> > how to extract the typedefs on OSX.
> >
> > Anyone running a buildfarm member should be able to do this and add to
> > the results, if they are up to date with release 3.2. I have my linux
> > crontab set up to do one typedefs run on the HEAD branch each day.
>
> [ Discussion deleted.]
>
> Andrew, this is disappointing news. When you talked about generating an
> typedef list from the buildfarm, you were saying how great it would be
> --- now a year later you post:
>
> It'd be nice to get that dealt with before we run pg_indent, but it's
> not like we'd be any worse off than before if we don't. In any case it's
> surely no blocker for 8.4beta.

My apologies; the above are Tom's words, not Andrews.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> bruce wrote:
>
>> Andrew Dunstan wrote:
>>
>>> This URL <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl> gives a
>>> typedef list that is (currently) the combined result from three fairly
>>> different buildfarm members:
>>>
>>> dungbeetle | 2009-03-22 06:44:01
>>> brown_bat | 2009-03-21 13:00:58
>>> dawn_bat | 2009-03-21 14:23:40
>>>
>>> These are respectively my Linux, Cygwin and MinGW buildfarm members.
>>>
>>> I don't have a BSD machine of any flavor to test on, and I don't know
>>> how to extract the typedefs on OSX.
>>>
>>> Anyone running a buildfarm member should be able to do this and add to
>>> the results, if they are up to date with release 3.2. I have my linux
>>> crontab set up to do one typedefs run on the HEAD branch each day.
>>>
>> [ Discussion deleted.]
>>
>> Andrew, this is disappointing news. When you talked about generating an
>> typedef list from the buildfarm, you were saying how great it would be
>> --- now a year later you post:
>>
>> It'd be nice to get that dealt with before we run pg_indent, but it's
>> not like we'd be any worse off than before if we don't. In any case it's
>> surely no blocker for 8.4beta.
>>
>
> My apologies; the above are Tom's words, not Andrews.
>
>

Apology accepted.

What I promised was a list that was more comprehensive than what we were
using. I think I've already delivered on that, but I would like to do
better by including some other Operating Systems: particularly some BSD
flavors. Buildfarm owners with non-Linux non-Windows members please
take note. Email me if you need help with this.

Unless we come up with some tolerably correct and maintainable code
analysis tool for identifying typedefs, using the current heuristic
methods is apparently the best we can do. Nobody has suggested even an
outline for such a tool. I don't think using the buildfarm for this
heuristic method is great, and never suggested it would be. I do think
it's an improvement, which is what I promised. I'm sorry if you find the
result disappointing.

cheers

andrew

Andrew Dunstan wrote:
> >> Andrew, this is disappointing news. When you talked about generating an
> >> typedef list from the buildfarm, you were saying how great it would be
> >> --- now a year later you post:
> >>
> >> It'd be nice to get that dealt with before we run pg_indent, but it's
> >> not like we'd be any worse off than before if we don't. In any case it's
> >> surely no blocker for 8.4beta.
> >>
> >
> > My apologies; the above are Tom's words, not Andrews.
> >
> >
>
> Apology accepted.
>
> What I promised was a list that was more comprehensive than what we were
> using. I think I've already delivered on that, but I would like to do
> better by including some other Operating Systems: particularly some BSD
> flavors. Buildfarm owners with non-Linux non-Windows members please
> take note. Email me if you need help with this.
>
> Unless we come up with some tolerably correct and maintainable code
> analysis tool for identifying typedefs, using the current heuristic
> methods is apparently the best we can do. Nobody has suggested even an
> outline for such a tool. I don't think using the buildfarm for this
> heuristic method is great, and never suggested it would be. I do think
> it's an improvement, which is what I promised. I'm sorry if you find the
> result disappointing.

Well, as you, I was hoping for a clear solution, and it seems we don't
have one. I think the false-positives problem is real and might make
the greater code coverage of the buildfarm worse than what we did for
8.3.

I think our only fallback is to find places that our BSD items miss,
perhaps Win32 cases, see what is different with those lists, and just
hard-code them in, because then we aren't importing a huge number of
additional typedefs that have uncertain consequences.

Frankly, I don't remember anyone complaining we didn't find any typedefs
in pgindent, though I think there might have been a few EXEC_BACKEND
cases, and maybe we can just hardcode those.

Frankly, pgindent has larger problems than an imcomplete typedef list. :-(

When I am ready to run pgindent I will ask for your typedef list and see
what the diff shows when I use your list and we can figure something out
then.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Frankly, I don't remember anyone complaining we didn't find any typedefs
> in pgindent,

There are lots and lots of places where it's obvious that pgindent
was misinformed on the subject, because it puts in a space where there
should not be one, eg "typename * ptr" instead of "typename *ptr".
Maybe I'm just being too anal in worrying about a space here or a space
there ... but then why do we run the thing at all?

This is the first time I've heard anyone suggest that false positives
could be a problem. What exactly would be the results of a false
match? Would it look worse than the false negatives do?

regards, tom lane

Bruce Momjian wrote:

> Well, as you, I was hoping for a clear solution, and it seems we don't
> have one. I think the false-positives problem is real and might make
> the greater code coverage of the buildfarm worse than what we did for
> 8.3.

Huh? What false positive problem?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Frankly, I don't remember anyone complaining we didn't find any typedefs
> > in pgindent,
>
> There are lots and lots of places where it's obvious that pgindent
> was misinformed on the subject, because it puts in a space where there
> should not be one, eg "typename * ptr" instead of "typename *ptr".
> Maybe I'm just being too anal in worrying about a space here or a space
> there ... but then why do we run the thing at all?

Sure, why not make it as good as we can.

> This is the first time I've heard anyone suggest that false positives
> could be a problem. What exactly would be the results of a false
> match? Would it look worse than the false negatives do?

Well, I assume a false positive would do the opposite, meaning it would
not have a space where it should have one. I am also worried about a
typedef list that changes from release to release as buildfarm members
change; variability might be worse than correctness in this case.

Anyway, I think a diff of using my list and Andrew's list will show us
which one gets things clearest; the diff is going to highlight only
cases where the typedef lists change formatting.

Andrew, where exactly is the list I should try?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > Well, as you, I was hoping for a clear solution, and it seems we don't
> > have one. I think the false-positives problem is real and might make
> > the greater code coverage of the buildfarm worse than what we did for
> > 8.3.
>
> Huh? What false positive problem?

typedefs listed on platforms that match identifiers in our code that are
_not_ typedefs.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> >
> > > Well, as you, I was hoping for a clear solution, and it seems we don't
> > > have one. I think the false-positives problem is real and might make
> > > the greater code coverage of the buildfarm worse than what we did for
> > > 8.3.
> >
> > Huh? What false positive problem?
>
> typedefs listed on platforms that match identifiers in our code that are
> _not_ typedefs.

Does this actually happen anywhere?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > Alvaro Herrera wrote:
> > > Bruce Momjian wrote:
> > >
> > > > Well, as you, I was hoping for a clear solution, and it seems we don't
> > > > have one. I think the false-positives problem is real and might make
> > > > the greater code coverage of the buildfarm worse than what we did for
> > > > 8.3.
> > >
> > > Huh? What false positive problem?
> >
> > typedefs listed on platforms that match identifiers in our code that are
> > _not_ typedefs.
>
> Does this actually happen anywhere?

No idea; it was more a theoretical issue to say that having more
typedefs is not necessarily a good thing; they should ideally be the
typedefs we use, and Windows adds a lot of typedefs we don't use.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Anyway, I think a diff of using my list and Andrew's list will show us
> which one gets things clearest; the diff is going to highlight only
> cases where the typedef lists change formatting.
>
> Andrew, where exactly is the list I should try?
>
>

fetch it from <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl>

cheers

andrew

Bruce Momjian wrote:
> Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> > > Alvaro Herrera wrote:

> > > > Huh? What false positive problem?
> > >
> > > typedefs listed on platforms that match identifiers in our code that are
> > > _not_ typedefs.
> >
> > Does this actually happen anywhere?
>
> No idea; it was more a theoretical issue to say that having more
> typedefs is not necessarily a good thing; they should ideally be the
> typedefs we use, and Windows adds a lot of typedefs we don't use.

Okay, so I went over the mingw list a bit (not exhaustively) and found
no typedef that's used as an identifier in our code.

Huh ... just found one. It's called "timezone", but it's used as an
identifier only in the function declaration (dt2local), not in the
function definition, which uses "tz" instead.

There's also ACL, but we only use it in macro definitions.

There are a bunch of other typedefs that the mingw port adds, but
several of them are actually used in our code (HANDLE, BOOL, etc).

I think this is minor enough that it should be ignored.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
> > Anyway, I think a diff of using my list and Andrew's list will show us
> > which one gets things clearest; the diff is going to highlight only
> > cases where the typedef lists change formatting.
> >
> > Andrew, where exactly is the list I should try?
> >
> >
>
> fetch it from <http://www.pgbuildfarm.org/cgi-bin/typedefs.pl>

Thanks. I will run tests when we are ready for pg_indent and we can
then make a decision.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

> Thanks. I will run tests when we are ready for pg_indent and we can
> then make a decision.

FWIW I was looking at this code for unrelated reasons and found a couple
of symbols that pgindent considers to be typedefs but it clearly are not
-- BufferHitCount and LocalBufferHitCount. It can be seen in
ShowBufferUsage().

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > Thanks. I will run tests when we are ready for pg_indent and we can
> > then make a decision.
>
> FWIW I was looking at this code for unrelated reasons and found a couple
> of symbols that pgindent considers to be typedefs but it clearly are not
> -- BufferHitCount and LocalBufferHitCount. It can be seen in
> ShowBufferUsage().

Are you saying you saw this in Andrew's typedef output, or from the 8.3
run of pg_indent?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> >
> > > Thanks. I will run tests when we are ready for pg_indent and we can
> > > then make a decision.
> >
> > FWIW I was looking at this code for unrelated reasons and found a couple
> > of symbols that pgindent considers to be typedefs but it clearly are not
> > -- BufferHitCount and LocalBufferHitCount. It can be seen in
> > ShowBufferUsage().
>
> Are you saying you saw this in Andrew's typedef output, or from the 8.3
> run of pg_indent?

This is on the 8.3 code. Notice how this is formatted:

hitrate = (float) BufferHitCount *100.0 / ReadBufferCount;

(line 1465)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
>> Alvaro Herrera wrote:
>>
>>> Bruce Momjian wrote:
>>>
>>>
>>>> Thanks. I will run tests when we are ready for pg_indent and we can
>>>> then make a decision.
>>>>
>>> FWIW I was looking at this code for unrelated reasons and found a couple
>>> of symbols that pgindent considers to be typedefs but it clearly are not
>>> -- BufferHitCount and LocalBufferHitCount. It can be seen in
>>> ShowBufferUsage().
>>>
>> Are you saying you saw this in Andrew's typedef output, or from the 8.3
>> run of pg_indent?
>>
>
> This is on the 8.3 code. Notice how this is formatted:
>
> hitrate = (float) BufferHitCount *100.0 / ReadBufferCount;
>
>
>
>

These symbols are not in the buildfarm's list of typedefs.

cheers

andrew

Alvaro Herrera wrote:

> This is on the 8.3 code. Notice how this is formatted:
>
> hitrate = (float) BufferHitCount *100.0 / ReadBufferCount;

Hmm, I just noticed that this is mentioned as a "known bug" in pgindent.
Nevermind ...

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andrew Dunstan wrote:
> >>> FWIW I was looking at this code for unrelated reasons and found a couple
> >>> of symbols that pgindent considers to be typedefs but it clearly are not
> >>> -- BufferHitCount and LocalBufferHitCount. It can be seen in
> >>> ShowBufferUsage().
> >>>
> >> Are you saying you saw this in Andrew's typedef output, or from the 8.3
> >> run of pg_indent?
> >>
> >
> > This is on the 8.3 code. Notice how this is formatted:
> >
> > hitrate = (float) BufferHitCount *100.0 / ReadBufferCount;
> >
> >
> >
> >
>
>
> These symbols are not in the buildfarm's list of typedefs.

The good news is that LocalBufferHitCount isn't in my list of typedefs
from CVS HEAD, and probably not in 8.3 either. The bad news is that
pgindent pushes the '*' next to the 100.0 in my testing. :-(

I tested BSD indent alone with no arguments or typedef list and got the
same output, even after adding the space in the C file, so something
wrong must be happening in the binary.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Andrew Dunstan wrote:
>> This is on the 8.3 code. Notice how this is formatted:
>>
>> hitrate = (float) BufferHitCount *100.0 / ReadBufferCount;

> The good news is that LocalBufferHitCount isn't in my list of typedefs
> from CVS HEAD, and probably not in 8.3 either. The bad news is that
> pgindent pushes the '*' next to the 100.0 in my testing. :-(

It's the (float), possibly in combination with the *, that does that.
There are many occurrences of this with other type names, eg (double).
I think it's too dumb to figure out that this is a cast and not a
variable declaration.

regards, tom lane