Clang compiler warning on 9.3 HEAD
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.
dependency.c:213:36: warning: implicit conversion from enumeration type
'ObjectClass' (aka 'enum ObjectClass') to different
enumeration type 'ObjectType' (aka 'enum ObjectType') [-Wconversion]
EventTriggerSupportsObjectType(getObjectClass(thisobj)))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
obj-type-class.patchtext/x-diff; charset=us-asciiDownload
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
***************
*** 210,216 **** deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
ObjectAddress *thisobj = targetObjects->refs + i;
if ((!(flags & PERFORM_DELETION_INTERNAL)) &&
! EventTriggerSupportsObjectType(getObjectClass(thisobj)))
{
EventTriggerSQLDropAddObject(thisobj);
}
--- 210,216 ----
ObjectAddress *thisobj = targetObjects->refs + i;
if ((!(flags & PERFORM_DELETION_INTERNAL)) &&
! EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
{
EventTriggerSQLDropAddObject(thisobj);
}
*** a/src/backend/commands/event_trigger.c
--- b/src/backend/commands/event_trigger.c
***************
*** 912,917 **** EventTriggerSupportsObjectType(ObjectType obtype)
--- 912,939 ----
}
/*
+ * Do event triggers support this object class?
+ */
+ bool
+ EventTriggerSupportsObjectClass(ObjectClass objclass)
+ {
+ switch (objclass)
+ {
+ case OCLASS_DATABASE:
+ case OCLASS_TBLSPACE:
+ case OCLASS_ROLE:
+ /* no support for global objects */
+ return false;
+ case OCLASS_EVENT_TRIGGER:
+ /* no support for event triggers on event triggers */
+ return false;
+ default:
+ break;
+ }
+ return true;
+ }
+
+ /*
* Prepare event trigger state for a new complete query to run, if necessary;
* returns whether this was done. If it was, EventTriggerEndCompleteQuery must
* be called when the query is done, regardless of whether it succeeds or fails
*** a/src/include/commands/event_trigger.h
--- b/src/include/commands/event_trigger.h
***************
*** 13,18 ****
--- 13,19 ----
#ifndef EVENT_TRIGGER_H
#define EVENT_TRIGGER_H
+ #include "catalog/dependency.h"
#include "catalog/objectaddress.h"
#include "catalog/pg_event_trigger.h"
#include "nodes/parsenodes.h"
***************
*** 41,46 **** extern Oid AlterEventTriggerOwner(const char *name, Oid newOwnerId);
--- 42,48 ----
extern void AlterEventTriggerOwner_oid(Oid, Oid newOwnerId);
extern bool EventTriggerSupportsObjectType(ObjectType obtype);
+ extern bool EventTriggerSupportsObjectClass(ObjectClass objclass);
extern void EventTriggerDDLCommandStart(Node *parsetree);
extern void EventTriggerDDLCommandEnd(Node *parsetree);
extern void EventTriggerSQLDrop(Node *parsetree);
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers