Обсуждение: [HACKERS] make more use of RoleSpec struct
Here is a small cleanup patch to make more use of the RoleSpec struct/node throughout the parser to avoid casts and make the code more self-documenting. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Вложения
Peter Eisentraut wrote: > Here is a small cleanup patch to make more use of the RoleSpec > struct/node throughout the parser to avoid casts and make the code more > self-documenting. This makes sense to me. I started to do this at some point when I was writing RoleSpec; eventually got annoyed about fixing the fallout (I was working to get somebody else's patch committed) and went back to how it is. Glad to see it done. The only functional issue might be the removal of the IsA() checks. If we don't cast any Node before passing it to any of those functions, there should be no problem because any misfeasance will be reported as a compile-time warning. Perhaps it's worth adding IsA() checks in loops such as the one in roleSpecsToIds(). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > Peter Eisentraut wrote: > > Here is a small cleanup patch to make more use of the RoleSpec > > struct/node throughout the parser to avoid casts and make the code more > > self-documenting. > > This makes sense to me. I started to do this at some point when I was > writing RoleSpec; eventually got annoyed about fixing the fallout (I was > working to get somebody else's patch committed) and went back to how it > is. Glad to see it done. > > The only functional issue might be the removal of the IsA() checks. If > we don't cast any Node before passing it to any of those functions, > there should be no problem because any misfeasance will be reported as a > compile-time warning. Perhaps it's worth adding IsA() checks in loops > such as the one in roleSpecsToIds(). Maybe inside of an Assert() though..? Thanks! Stephen
Stephen Frost wrote: > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > The only functional issue might be the removal of the IsA() checks. If > > we don't cast any Node before passing it to any of those functions, > > there should be no problem because any misfeasance will be reported as a > > compile-time warning. Perhaps it's worth adding IsA() checks in loops > > such as the one in roleSpecsToIds(). > > Maybe inside of an Assert() though..? Yeah, I was of two minds when writing that para. Maybe roleSpecsToIds (and all similar functions, if any others exist) has a limited enough caller base that it's trivial to audit that no bogus callsite exists. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > > > The only functional issue might be the removal of the IsA() checks. If > > > we don't cast any Node before passing it to any of those functions, > > > there should be no problem because any misfeasance will be reported as a > > > compile-time warning. Perhaps it's worth adding IsA() checks in loops > > > such as the one in roleSpecsToIds(). > > > > Maybe inside of an Assert() though..? > > Yeah, I was of two minds when writing that para. Maybe roleSpecsToIds > (and all similar functions, if any others exist) has a limited enough > caller base that it's trivial to audit that no bogus callsite exists. Well, I think an Assert() is fine to make sure someone doesn't add a new call that passes in something else where we don't have the compile-time type checking happening. Thanks! Stephen
On 12/28/16 9:53 AM, Alvaro Herrera wrote: > Peter Eisentraut wrote: >> Here is a small cleanup patch to make more use of the RoleSpec >> struct/node throughout the parser to avoid casts and make the code more >> self-documenting. > > This makes sense to me. I started to do this at some point when I was > writing RoleSpec; eventually got annoyed about fixing the fallout (I was > working to get somebody else's patch committed) and went back to how it > is. Glad to see it done. > > The only functional issue might be the removal of the IsA() checks. If > we don't cast any Node before passing it to any of those functions, > there should be no problem because any misfeasance will be reported as a > compile-time warning. Perhaps it's worth adding IsA() checks in loops > such as the one in roleSpecsToIds(). Committed with an extra Assert there for good measure. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services