Thread: pgsql: Remove useless default clause in switch
Remove useless default clause in switch The switch covers all values of the enum driver variable, so having a default: clause is useless, even if it's only to do Assert(false). Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/dfce1f9e4eef3adcccbb23670fa1c432eebb0b90 Modified Files -------------- src/backend/partitioning/partprune.c | 4 ---- 1 file changed, 4 deletions(-)
On 24 April 2018 at 03:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Remove useless default clause in switch > > The switch covers all values of the enum driver variable, so having a > default: clause is useless, even if it's only to do Assert(false). Just for my own understanding: I always thought that when all options were covered that we generally kept a default just in case someone added another enum and forgot to update the code. I know generally those are with elog ERRORs but both would be designed to alert a programmer, just at different times. There are other examples in that file with the switch (part_scheme->strategy), these are not using enums. I'd have to assume that these must be different because of that. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
> On 24 April 2018 at 03:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Remove useless default clause in switch
> I always thought that when all options were covered that we generally
> kept a default just in case someone added another enum and forgot to
> update the code.
Actually, there's an advantage to omitting the default for enum-based
switches, which is that many compilers will give you a compile-time
warning that you forgot a case. That beats an elog(ERROR) that you
might or might not hit in testing.
> There are other examples in that file with the switch
> (part_scheme->strategy), these are not using enums.
If it's not based on an enum then the compiler isn't going to understand
that you missed a value, so having the elog is better than having nothing.
regards, tom lane
>>>>> "David" == David Rowley <david.rowley@2ndquadrant.com> writes: David> I always thought that when all options were covered that we David> generally kept a default just in case someone added another enum David> and forgot to update the code. That's what compiler warnings (-Wswitch, included in -Wall) are for; it's actually a _bad_ idea to have a default clause in such cases, since it stops the compiler from warning you. -- Andrew (irc:RhodiumToad)
On Tue, Apr 24, 2018 at 12:27:27PM +1200, David Rowley wrote: > I always thought that when all options were covered that we generally > kept a default just in case someone added another enum and forgot to > update the code. > > I know generally those are with elog ERRORs but both would be designed > to alert a programmer, just at different times. The advantage with what Alvaro has done is that you get warning at compilation time, so it is much more helpful for developers when adding an option because they need to think about code paths where the warning comes from. > There are other examples in that file with the switch > (part_scheme->strategy), these are not using enums. I'd have to assume > that these must be different because of that. Those apply to PARTITION_STRATEGY_LIST and such, which are not defined based on an enumeration but using their catalog interpretation, so this assumption cannot apply. The same applies for BTLessEqualStrategyNumber & friends which have their own definition. You are right for what's in perform_pruning_combine_step though, so the attached could also be applied. -- Michael
Attachment
On 2018/04/24 10:20, Michael Paquier wrote: > On Tue, Apr 24, 2018 at 12:27:27PM +1200, David Rowley wrote: >> I always thought that when all options were covered that we generally >> kept a default just in case someone added another enum and forgot to >> update the code. >> >> I know generally those are with elog ERRORs but both would be designed >> to alert a programmer, just at different times. > > The advantage with what Alvaro has done is that you get warning at > compilation time, so it is much more helpful for developers when > adding an option because they need to think about code paths where the > warning comes from. +1 >> There are other examples in that file with the switch >> (part_scheme->strategy), these are not using enums. I'd have to assume >> that these must be different because of that. > > Those apply to PARTITION_STRATEGY_LIST and such, which are not defined > based on an enumeration but using their catalog interpretation, so this > assumption cannot apply. The same applies for BTLessEqualStrategyNumber > & friends which have their own definition. > > You are right for what's in perform_pruning_combine_step though, so the > attached could also be applied. Thanks for spotting that one. Regards, Amit
David Rowley wrote: > On 24 April 2018 at 03:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Remove useless default clause in switch > > > > The switch covers all values of the enum driver variable, so having a > > default: clause is useless, even if it's only to do Assert(false). > > Just for my own understanding: > > I always thought that when all options were covered that we generally > kept a default just in case someone added another enum and forgot to > update the code. The compiler will emit a warning when it sees that not all cases are handled, so it's not necessary to have a default case. > There are other examples in that file with the switch > (part_scheme->strategy), these are not using enums. I'd have to assume > that these must be different because of that. Yeah, operator strategy numbers are a hopeless case, because each index AM defines its own set of valid strategy numbers. Another interesting case is PARTITION_STRATEGY which I wanted to convert to an enum, but got sidetracked. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 24 April 2018 at 13:50, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > David Rowley wrote: >> On 24 April 2018 at 03:12, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> > Remove useless default clause in switch >> > >> > The switch covers all values of the enum driver variable, so having a >> > default: clause is useless, even if it's only to do Assert(false). >> >> Just for my own understanding: >> >> I always thought that when all options were covered that we generally >> kept a default just in case someone added another enum and forgot to >> update the code. > > The compiler will emit a warning when it sees that not all cases are > handled, so it's not necessary to have a default case. Thank you all for explaining that. It very much makes sense now. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services