Make it a compile error to use a crash key with no variable name.

base/debug/crash_logging.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.
 
 #ifndef BASE_DEBUG_CRASH_LOGGING_H_
 #define BASE_DEBUG_CRASH_LOGGING_H_
 
 #include <stddef.h>
 
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 
 // These functions add metadata to the upload payload when sending crash reports
 // to the crash server.
 //
 // IMPORTANT: On OS X and Linux, the key/value pairs are only sent as part of
@@ skipping 14 matching lines @@
 BASE_EXPORT void SetCrashKeyToStackTrace(const base::StringPiece& key,
                                          const StackTrace& trace);
 
 // Formats |count| instruction pointers from |addresses| using %p and
 // sets the resulting string as a value for crash key |key|. A maximum of 23
 // items will be encoded, since breakpad limits values to 255 bytes.
 BASE_EXPORT void SetCrashKeyFromAddresses(const base::StringPiece& key,
                                           const void* const* addresses,
                                           size_t count);
 
+enum ScopedCrashKeyNeedsNameTag {

[danakj, 2016/07/19 21:02:57]

nit: I'd nest this inside ScopedCrashKey, beside (right below?) the nocompile
constructor.

[dcheng, 2016/07/22 05:39:22]

Done.

+  SCOPED_CRASH_KEY_NEEDS_NAME,
+};
+
 // A scoper that sets the specified key to value for the lifetime of the
 // object, and clears it on destruction.
 class BASE_EXPORT ScopedCrashKey {
  public:
   ScopedCrashKey(const base::StringPiece& key, const base::StringPiece& value);
   ~ScopedCrashKey();
 
+  // Helper to force a static_assert when instantiating a ScopedCrashKey
+  // temporary without a name. The usual idiom is to just #define a macro that
+  // static_asserts with the message; however, that doesn't work well when the
+  // type is in a namespace.
+  //
+  // Instead, we use a templated helper to trigger the static_assert, observing
+  //   two rules:
+  // - The static_assert needs to be in a normally uninstantiated template;
+  //   otherwise, it will fail to compile =)
+  // - Similarly, the static_assert must be dependent on the template argument,
+  //   to prevent it from being evaluated until the template is instantiated.
+  //
+  // To prevent this constructor from being accidentally invoked, it takes a
+  // special enum as an argument.
+
+  // TODO(dcheng): For reasons unknown to me, we can't make this a template
+  // function taking just one parameter (the enum tag). While the build
+  // correctly fails, the static_assert is never triggered. I think this is
+  // because SFINAE removes the overload from consideration before it's ever
+  // instantiated, but I'm not sure...
+  template <typename... Args>
+  explicit ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) {

[tzik, 2016/07/14 07:39:53]

Can we just disable this by "= delete"?

[dcheng, 2016/07/14 07:41:15]

We can, but the idea is to provide a useful error in the build log. With =
delete, this will just complain that we tried to call a deleted function. With
static_assert, the output looks like this:

e:\b\build\slave\win\build\src\base\debug\crash_logging.h(82): error C2338:
scoped crash key objects should not be unnamed temporaries.

[tzik, 2016/07/14 07:45:41]

Ok, so. How about define this:
template <typename...>
struct true_t : std::true_type {};
... and use this as
static_assert(true_t<Args...>::value, "...");
?

+    constexpr bool always_false = sizeof...(Args) == 0 && sizeof...(Args) != 0;
+    static_assert(
+        always_false,
+        "scoped crash key objects should not be unnamed temporaries.");
+  }
+
  private:
   std::string key_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedCrashKey);
 };
 
+// Disallow an instantation of ScopedCrashKey without a name, since this results
+// in a temporary that is immediately destroyed. Doing so will trigger the
+// static_assert in the templated constructor helper in ScopedCrashKey.
+#define ScopedCrashKey(...) \
+  ScopedCrashKey(base::debug::SCOPED_CRASH_KEY_NEEDS_NAME, __VA_ARGS__)
+
 // Before setting values for a key, all the keys must be registered.
 struct BASE_EXPORT CrashKey {
   // The name of the crash key, used in the above functions.
   const char* key_name;
 
   // The maximum length for a value. If the value is longer than this, it will
   // be truncated. If the value is larger than the |chunk_max_length| passed to
   // InitCrashKeys() but less than this value, it will be split into multiple
   // numbered chunks.
   size_t max_length;
@@ skipping 29 matching lines @@
     const base::StringPiece& value,
     size_t chunk_max_length);
 
 // Resets the crash key system so it can be reinitialized. For testing only.
 BASE_EXPORT void ResetCrashLoggingForTesting();
 
 }  // namespace debug
 }  // namespace base
 
 #endif  // BASE_DEBUG_CRASH_LOGGING_H_