Clarify that BlockSizeForFileType can't be called with EXTERNAL FileType

net/disk_cache/blockfile/addr.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This is an internal class that handles the address of a cache record.
 // See net/disk_cache/disk_cache.h for the public interface of the cache.
 
 #ifndef NET_DISK_CACHE_BLOCKFILE_ADDR_H_
 #define NET_DISK_CACHE_BLOCKFILE_ADDR_H_
 
 #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"
 
 namespace disk_cache {
 
 enum FileType {
   EXTERNAL = 0,
   RANKINGS = 1,
   BLOCK_256 = 2,
   BLOCK_1K = 3,
@@ skipping 107 matching lines @@
       case BLOCK_1K:
         return 1024;
       case BLOCK_4K:
         return 4096;
       case BLOCK_FILES:
         return 8;
       case BLOCK_ENTRIES:
         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;
     }
   }
 
   static FileType RequiredFileType(int size) {
     if (size < 1024)
       return BLOCK_256;
     else if (size < 4096)
       return BLOCK_1K;
     else if (size <= 4096 * 4)
@@ skipping 25 matching lines @@
   static const uint32_t kFileSelectorOffset = 16;
   static const uint32_t kStartBlockMask = 0x0000FFFF;
   static const uint32_t kFileNameMask = 0x0FFFFFFF;
 
   CacheAddr value_;
 };
 
 }  // namespace disk_cache
 
 #endif  // NET_DISK_CACHE_BLOCKFILE_ADDR_H_