Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

net/disk_cache/blockfile/addr.h

Index: net/disk_cache/blockfile/addr.h
diff --git a/net/disk_cache/blockfile/addr.h b/net/disk_cache/blockfile/addr.h
index f21fbf2aa14000ccdd6f978a976ed238b4bc3099..e55499caa48e42fc9b6ebd727fd2449f6ca6f020 100644
--- a/net/disk_cache/blockfile/addr.h
+++ b/net/disk_cache/blockfile/addr.h
@@ -11,6 +11,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include "base/logging.h"
 #include "net/base/net_export.h"
 #include "net/disk_cache/blockfile/disk_format_base.h"
 
@@ -138,7 +139,8 @@ class NET_EXPORT_PRIVATE Addr {
         return 104;
       case BLOCK_EVICTED:
         return 48;
-      default:
+      case EXTERNAL:
+        NOTREACHED();

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

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]

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]

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:35]

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]

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

         return 0;
     }
   }