Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198
Committed: https://crrev.com/d8c29e03c348bdb7ce8522f22515523de1c20df9
Cr-Commit-Position: refs/heads/master@{#428279}

[hans, 2016-10-27 23:09:05]

hans@chromium.org changed reviewers:
+ zhongyi@chromium.org

[hans, 2016-10-27 23:09:05]

Please take a look.

[Zhongyi Shi, 2016-10-27 23:13:27]

Could you update your issue description? It seems inconsistent with the code
change to me.

[hans, 2016-10-27 23:18:35]

On 2016/10/27 23:13:27, Zhongyi Shi wrote:
> Could you update your issue description? It seems inconsistent with the code
> change to me.

Hmm, how is it inconsistent? My change is doing exactly what the description
says I think :-)

Adding "case EXTERNAL: NOTREACHED()" spells out that the function can't be
called with that file type. Since there are no other file types possible, there
is no need for the default:.

I guess "and cover the enum" should be "because the enum is now covered" though.
I'll change that.

[hans, 2016-10-27 23:18:57]

Description was changed from

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case and cover the enum. Clang warns if the
switch doesn't cover all enum values.

BUG=660198
==========

to

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198
==========

[Zhongyi Shi, 2016-10-27 23:19:00]

On 2016/10/27 23:13:27, Zhongyi Shi wrote:
> Could you update your issue description? It seems inconsistent with the code
> change to me.

Whoops, I misread the description :(

[Zhongyi Shi, 2016-10-27 23:29:43]

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
File net/disk_cache/blockfile/addr.h (right):

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
net/disk_cache/blockfile/addr.h:143: NOTREACHED();
nit: I would prefer to have the default case but with a NOTREACHED() check, you
could add a comment then saying EXERNAL case shouldn't called in this case.

[hans, 2016-10-27 23:33:35]

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
File net/disk_cache/blockfile/addr.h (right):

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
net/disk_cache/blockfile/addr.h:143: NOTREACHED();
On 2016/10/27 23:29:43, Zhongyi Shi wrote:
> nit: I would prefer to have the default case but with a NOTREACHED() check,
you
> could add a comment then saying EXERNAL case shouldn't called in this case. 

But then you won't get any compiler warning if another enum value is added and
not handled in the switch.

[Zhongyi Shi, 2016-10-28 00:01:49]

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
File net/disk_cache/blockfile/addr.h (right):

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
net/disk_cache/blockfile/addr.h:143: NOTREACHED();
On 2016/10/27 23:33:35, hans wrote:
> On 2016/10/27 23:29:43, Zhongyi Shi wrote:
> > nit: I would prefer to have the default case but with a NOTREACHED() check,
> you
> > could add a comment then saying EXERNAL case shouldn't called in this case. 
> 
> But then you won't get any compiler warning if another enum value is added and
> not handled in the switch.

What I am proposing is

case BLOCK_EVICTED:
  return 48;
default:
  // EXTERNAL shouldn't be allowed to call this method. 
  NOTREACHED();
  return 0;

Are you trying to enforce every enum type to be handled in this method even if
it's never called? If it is going to be called while the enum is not handled,
the NOTREACH() will fire in the default case. Redundantly, though I will be
supportive of, you could also have both CASE EXTERNAL and default to have a
NOTREACHED() check.

[hans, 2016-10-28 00:57:36]

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
File net/disk_cache/blockfile/addr.h (right):

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
net/disk_cache/blockfile/addr.h:143: NOTREACHED();
On 2016/10/28 00:01:49, Zhongyi Shi wrote:
> On 2016/10/27 23:33:35, hans wrote:
> > On 2016/10/27 23:29:43, Zhongyi Shi wrote:
> > > nit: I would prefer to have the default case but with a NOTREACHED()
check,
> > you
> > > could add a comment then saying EXERNAL case shouldn't called in this
case. 
> > 
> > But then you won't get any compiler warning if another enum value is added
and
> > not handled in the switch.
> 
> What I am proposing is
> 
> case BLOCK_EVICTED:
>   return 48;
> default:
>   // EXTERNAL shouldn't be allowed to call this method. 
>   NOTREACHED();
>   return 0;
> 
> Are you trying to enforce every enum type to be handled in this method even if
> it's never called? If it is going to be called while the enum is not handled,
> the NOTREACH() will fire in the default case. Redundantly, though I will be
> supportive of, you could also have both CASE EXTERNAL and default to have a
> NOTREACHED() check.

Yes, every enum needs to be handled in the switch, otherwise Clang will warn:

../../net/disk_cache/blockfile/addr.h:127:13: error: enumeration value 'FOO' not
handled in switch [-Werror,-Wswitch]
    switch (file_type) {
            ^

The easiest way to defeat the warning is of course to have a default case.

But not having a default case has the benefit that if a new enumeration value is
ever added, we will get a warning if it's not handled in this switch. If we have
a default case instead, we'd never know until possibly at runtime.


While your suggestion works:

default:
  // EXTERNAL shouldn't be allowed to call this method. 
  NOTREACHED();
  return 0;

I think it's slightly less clear. How does the reader know that EXTERNAL is the
only type that will end up in the default: case?

[Zhongyi Shi, 2016-10-28 02:26:49]

lgtm

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
File net/disk_cache/blockfile/addr.h (right):

https://codereview.chromium.org/2458773003/diff/1/net/disk_cache/blockfile/ad...
net/disk_cache/blockfile/addr.h:143: NOTREACHED();
On 2016/10/28 00:57:35, hans wrote:
> On 2016/10/28 00:01:49, Zhongyi Shi wrote:
> > On 2016/10/27 23:33:35, hans wrote:
> > > On 2016/10/27 23:29:43, Zhongyi Shi wrote:
> > > > nit: I would prefer to have the default case but with a NOTREACHED()
> check,
> > > you
> > > > could add a comment then saying EXERNAL case shouldn't called in this
> case. 
> > > 
> > > But then you won't get any compiler warning if another enum value is added
> and
> > > not handled in the switch.
> > 
> > What I am proposing is
> > 
> > case BLOCK_EVICTED:
> >   return 48;
> > default:
> >   // EXTERNAL shouldn't be allowed to call this method. 
> >   NOTREACHED();
> >   return 0;
> > 
> > Are you trying to enforce every enum type to be handled in this method even
if
> > it's never called? If it is going to be called while the enum is not
handled,
> > the NOTREACH() will fire in the default case. Redundantly, though I will be
> > supportive of, you could also have both CASE EXTERNAL and default to have a
> > NOTREACHED() check.
> 
> Yes, every enum needs to be handled in the switch, otherwise Clang will warn:
> 
> ../../net/disk_cache/blockfile/addr.h:127:13: error: enumeration value 'FOO'
not
> handled in switch [-Werror,-Wswitch]
>     switch (file_type) {
>             ^
> 
> The easiest way to defeat the warning is of course to have a default case.
> 
> But not having a default case has the benefit that if a new enumeration value
is
> ever added, we will get a warning if it's not handled in this switch. If we
have
> a default case instead, we'd never know until possibly at runtime.
> 
> 
> While your suggestion works:
> 
> default:
>   // EXTERNAL shouldn't be allowed to call this method. 
>   NOTREACHED();
>   return 0;
> 
> I think it's slightly less clear. How does the reader know that EXTERNAL is
the
> only type that will end up in the default: case?

Alright, your argument sold, I agreed that this could enforce handling of new
switch type.

[hans, 2016-10-28 02:35:05]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:35:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 02:47:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:47:24]

Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[hans, 2016-10-28 02:57:26]

The CQ bit was checked by hans@chromium.org

[hans, 2016-10-28 02:57:27]

The patchset sent to the CQ was uploaded after l-g-t-m from zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2458773003/#ps20001
(title: "pacify gcc")

[commit-bot: I haz the power, 2016-10-28 02:57:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 04:07:50]

Description was changed from

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198
==========

to

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198
==========

[commit-bot: I haz the power, 2016-10-28 04:07:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-28 04:09:34]

Description was changed from

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198
==========

to

==========
Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

PVS-Studio analysis pointed out that if it were called with such a file
type, that would lead to division by zero in RequiredBlocks.

Also, remove the default: case since the enum is now covered. Clang warns
if the switch doesn't cover all enum values.

BUG=660198

Committed: https://crrev.com/d8c29e03c348bdb7ce8522f22515523de1c20df9
Cr-Commit-Position: refs/heads/master@{#428279}
==========

[commit-bot: I haz the power, 2016-10-28 04:09:36]

Patchset 2 (id:??) landed as
https://crrev.com/d8c29e03c348bdb7ce8522f22515523de1c20df9
Cr-Commit-Position: refs/heads/master@{#428279}