Thread: Clang compiler warning on 9.3 HEAD

On Wed, Apr 3, 2013 at 11:52 PM, Will Leinweber <will@heroku.com> wrote:
> On ref 8507907 when compiling with clang on os x, I got this warning which
> seems like a possible bug.
>
> I thought to report this because I imagine clang isn't frequently used
> day-to-day by most.

I actually reported a bug that was thrown up by Clang for similar
reasons over a year ago (i.e. an implicit conversion of one enum type
to another). That bug was fixed (by this commit:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=05e83968929f4ec1eba058fcae755fd2df98864e
), and the diagnostics were considered generally sound and useful at
the time. This looks like a real problem to me.


-- 
Peter Geoghegan

Will Leinweber wrote:
> On ref 8507907 when compiling with clang on os x, I got this warning which
> seems like a possible bug.
>
> I thought to report this because I imagine clang isn't frequently used
> day-to-day by most.

Ugh.  My fault.  Yes, this is a bug.

I don't see any nice way to convert ObjectType to ObjectClass or the
other way around; it seems to me the only simple way to fix this is to
add a new function EventTriggerSupportsObjectClass(), which takes
ObjectClass instead of ObjectType.  Patch attached.

Now, it annoys me that we now have three places that know about object
types supported by event triggers: there's a large struct of command tag
substrings (event_trigger_support), then there's these two functions.
It might be better to add ObjectType and ObjectClass entries to the
struct, so that only the struct needs to know about that.  The problem
is that these two functions would have to walk the struct every time,
instead of being a simple switch.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Now, it annoys me that we now have three places that know about object
> types supported by event triggers: there's a large struct of command tag
> substrings (event_trigger_support), then there's these two functions.
> It might be better to add ObjectType and ObjectClass entries to the
> struct, so that only the struct needs to know about that.  The problem
> is that these two functions would have to walk the struct every time,
> instead of being a simple switch.

Yeah.  For the moment I think efficiency trumps having the information
in just one place, ie I agree with doing it like this.  If we find we're
constantly forgetting to fix the functions when we need to, it'd be time
enough to revisit the decision.

One thing you could do that might make it less likely we'd miss things
is to have the switches explicitly cover all the possible enum members,
instead of defaulting the majority of them as you do here.  I know when
I think about adding a new object type, I tend to choose the most
similar existing type and grep for references to it.  Likely you could
even draft the code so that most compilers would warn about an enum
value not covered in the switch.
        regards, tom lane

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Now, it annoys me that we now have three places that know about object
> types supported by event triggers: there's a large struct of command tag
> substrings (event_trigger_support), then there's these two functions.
> It might be better to add ObjectType and ObjectClass entries to the
> struct, so that only the struct needs to know about that.  The problem
> is that these two functions would have to walk the struct every time,
> instead of being a simple switch.

Back when I added an dedicated event per command, Robert asked me to
work on such a big struct containing all the parameters in the same
place. Then we got back to only a couple of events, and completely
forgot about that.

You can have a look at how it did look like here:
 https://github.com/dimitri/postgres/blob/evt_trig_v1/src/backend/utils/cache/evtcache.c

And rather than walk the struct, I did install a couple of dedicated
hash tables so that you could do direct and fast lookups.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Now, it annoys me that we now have three places that know about object
> > types supported by event triggers: there's a large struct of command tag
> > substrings (event_trigger_support), then there's these two functions.
> > It might be better to add ObjectType and ObjectClass entries to the
> > struct, so that only the struct needs to know about that.  The problem
> > is that these two functions would have to walk the struct every time,
> > instead of being a simple switch.
>
> Yeah.  For the moment I think efficiency trumps having the information
> in just one place, ie I agree with doing it like this.  If we find we're
> constantly forgetting to fix the functions when we need to, it'd be time
> enough to revisit the decision.

Makes sense.

> One thing you could do that might make it less likely we'd miss things
> is to have the switches explicitly cover all the possible enum members,
> instead of defaulting the majority of them as you do here.  I know when
> I think about adding a new object type, I tend to choose the most
> similar existing type and grep for references to it.  Likely you could
> even draft the code so that most compilers would warn about an enum
> value not covered in the switch.

I removed the default case, and my compiler is quiet about the current
coding and makes noise if I add a new value to the enums.  So I guess
we're covered.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services