pgsql: Remove useless default clause in switch

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

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