Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2137863002:
Make it a compile error to use a crash key with no variable name. (Closed)
DescriptionMake it a compile error to use a crash key with no variable name.
BUG=
Committed: https://crrev.com/288f45d2de18e1668f03b2eee51fee8cd7ae2fad
Cr-Commit-Position: refs/heads/master@{#407083}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Better, but not best #Patch Set 3 : Magic #Patch Set 4 : Add comments #
Total comments: 4
Patch Set 5 : . #Patch Set 6 : . #
Total comments: 9
Patch Set 7 : . #Messages
Total messages: 32 (7 generated)
dcheng@chromium.org changed reviewers: + danakj@chromium.org
Macros are bad. On the other hand, this has shown up twice recently and is quite easy to miss in review: - https://codereview.chromium.org/2101283002/ - https://codereview.chromium.org/2134623002/ So it's probably worth doing this for ScopedCrashKey, since the cost of missing this is somewhat high? https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h#... base/debug/crash_logging.h:58: #define ScopedCrashKey(x) static_assert(false, "a scoped crash key has no name") I'll think of a better error if people think this patch is OK. My previous message was 81 characters >_<
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h#... base/debug/crash_logging.h:58: #define ScopedCrashKey(x) static_assert(false, "a scoped crash key has no name") On 2016/07/11 01:42:44, dcheng wrote: > I'll think of a better error if people think this patch is OK. My previous > message was 81 characters >_< How does this work with only one arg?
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
This is kind of gross, but it would be good to detect anonymous objects. Would this also work in the WTF version?
On 2016/07/11 17:11:45, Robert Sesek wrote: > This is kind of gross, but it would be good to detect anonymous objects. Would > this also work in the WTF version? Yes, I believe this Just Works for the WTF version (the casing is the same and we include the //base header in wtf, so it just all magically propagates through the system)
https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/1/base/debug/crash_logging.h#... base/debug/crash_logging.h:58: #define ScopedCrashKey(x) static_assert(false, "a scoped crash key has no name") On 2016/07/11 17:08:12, ncarter wrote: > On 2016/07/11 01:42:44, dcheng wrote: > > I'll think of a better error if people think this patch is OK. My previous > > message was 81 characters >_< > > How does this work with only one arg? I've updated this to add the appropriate number of args. It still worked (but it complained about mismatched arg counts, rather than firing the static_assert). Unfortunately, since this is in a namespace, it still doesn't work quite as expected (clang complains about expecting an unqualified id). I have another idea, but it's a bit ugly... I'll upload it once I get it working.
OK, added some magic to make the static_assert show up everywhere. It abuses a new templated constructor: note that it's important that the static_assert is dependent on T; otherwise, it's undefined behavior.
On 2016/07/12 01:41:26, dcheng wrote: > OK, added some magic to make the static_assert show up everywhere. It abuses a > new templated constructor: note that it's important that the static_assert is > dependent on T; otherwise, it's undefined behavior. I'm okay with a magic, but could you add a comment to elaborate why/how we use the magic?
On 2016/07/12 05:32:04, Yuki wrote: > On 2016/07/12 01:41:26, dcheng wrote: > > OK, added some magic to make the static_assert show up everywhere. It abuses a > > new templated constructor: note that it's important that the static_assert is > > dependent on T; otherwise, it's undefined behavior. > > I'm okay with a magic, but could you add a comment to elaborate why/how we use > the magic? Added some comments to clarify how it works.
https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_loggin... base/debug/crash_logging.h:64: ScopedCrashKey(const T&) { Why a constructor and not just a free CrashFromBadScopedCrashKey() function or something?
https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_loggin... base/debug/crash_logging.h:64: ScopedCrashKey(const T&) { On 2016/07/12 19:59:26, danakj wrote: > Why a constructor and not just a free CrashFromBadScopedCrashKey() function or > something? This prevents me from having to "using base::debug::HelperFunction" in Blink.
https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_loggin... base/debug/crash_logging.h:64: ScopedCrashKey(const T&) { On 2016/07/13 00:34:11, dcheng wrote: > On 2016/07/12 19:59:26, danakj wrote: > > Why a constructor and not just a free CrashFromBadScopedCrashKey() function or > > something? > > This prevents me from having to "using base::debug::HelperFunction" in Blink. What about something like this: enum class UnnamedScopedCrashKey { ERROR }; template<typename T> ScopedCrashKey(const T&) { static_assert(std::is_same<T, UnnamedScopedCrashKey>::value == false, "Unnamed ScopedCrashKey"); static_assert(std::is_same<T, UnnamedScopedCrashKey>::value == true, "Invalid constructor"); }
https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/60001/base/debug/crash_loggin... base/debug/crash_logging.h:64: ScopedCrashKey(const T&) { On 2016/07/13 19:46:22, Robert Sesek wrote: > On 2016/07/13 00:34:11, dcheng wrote: > > On 2016/07/12 19:59:26, danakj wrote: > > > Why a constructor and not just a free CrashFromBadScopedCrashKey() function > or > > > something? > > > > This prevents me from having to "using base::debug::HelperFunction" in Blink. > > What about something like this: > > enum class UnnamedScopedCrashKey { ERROR }; > template<typename T> > ScopedCrashKey(const T&) { > static_assert(std::is_same<T, UnnamedScopedCrashKey>::value == false, "Unnamed > ScopedCrashKey"); > static_assert(std::is_same<T, UnnamedScopedCrashKey>::value == true, "Invalid > constructor"); > } I was just thinking similar things.. but more like enum class UnnamedScopedCrashKey { ERROR }; template<typename T> explicit ScopedCrashKey(const T&, UnnamedScopedCrashKey) { static_assert(...); } So that people can't really call this constructor as easily, they'd have to really try to.
OK, I managed to convince it to work with the enum tag but it took a fair amount of convincing. And I'm not entirely sure why I need to write it like this: in PS5, I tried the simpler form, and while the build still breaks, it breaks in an unhelpful way. If anyone else understands, please let me know... otherwise, I'm hoping not to futz around with weird C++ things much longer =P
tzik@chromium.org changed reviewers: + tzik@chromium.org
https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.h:78: explicit ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) { Can we just disable this by "= delete"?
https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.h:78: explicit ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) { On 2016/07/14 07:39:53, tzik wrote: > Can we just disable this by "= delete"? 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.
https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.h:78: explicit ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) { On 2016/07/14 07:41:15, dcheng wrote: > On 2016/07/14 07:39:53, tzik wrote: > > Can we just disable this by "= delete"? > > 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. Ok, so. How about define this: template <typename...> struct true_t : std::true_type {}; ... and use this as static_assert(true_t<Args...>::value, "..."); ?
On 2016/07/14 07:45:41, tzik wrote: > https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... > File base/debug/crash_logging.h (right): > > https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... > base/debug/crash_logging.h:78: explicit > ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) { > On 2016/07/14 07:41:15, dcheng wrote: > > On 2016/07/14 07:39:53, tzik wrote: > > > Can we just disable this by "= delete"? > > > > 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. > > Ok, so. How about define this: > template <typename...> > struct true_t : std::true_type {}; > ... and use this as > static_assert(true_t<Args...>::value, "..."); > ? I'm not sure I understand: how is this different from the current implementation? The current static_assert works; it would be nice to not need the extra parameters, but I can't figure out a way to do that.
On 2016/07/14 11:51:05, dcheng wrote: > On 2016/07/14 07:45:41, tzik wrote: > > > https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... > > File base/debug/crash_logging.h (right): > > > > > https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... > > base/debug/crash_logging.h:78: explicit > > ScopedCrashKey(ScopedCrashKeyNeedsNameTag, const Args&...) { > > On 2016/07/14 07:41:15, dcheng wrote: > > > On 2016/07/14 07:39:53, tzik wrote: > > > > Can we just disable this by "= delete"? > > > > > > 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. > > > > Ok, so. How about define this: > > template <typename...> > > struct true_t : std::true_type {}; > > ... and use this as > > static_assert(true_t<Args...>::value, "..."); > > ? > > I'm not sure I understand: how is this different from the current > implementation? The current static_assert works; it would be nice to not need > the extra parameters, but I can't figure out a way to do that. Ah, now I understand the situation. The red bots indicates successful assertion instead of failure, right? So there's no need to change the current one. Sorry for my noise. And to make extra noise from my understanding: We need to evaluate the static_assert() on the template instantiation instead of its definition. However, if we write the condition directly as static_assert(false), it fails on its definition phase. Also on gcc, simple expression is expanded on the definition phase. To prevent the evaluation on the template definition, we can use a (constexpr) function or a struct template that parametrized by the template parameter. Since it's unknown at the definition phase whether there are function overloads or template specializations, the compiler can not evaluate the assertion beyond the function call or the struct template.
Seems okay to me. One question: https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.cc (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.cc:16: #undef ScopedCrashKey The precompiler will replace the constructor defn with the macro otherwise?
https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.cc (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.cc:16: #undef ScopedCrashKey On 2016/07/19 02:14:05, danakj wrote: > The precompiler will replace the constructor defn with the macro otherwise? Yeah, this will garble the ctor. So this is a dumb hack to make it not shoot itself in the foot.
LGTM https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.cc (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.cc:16: #undef ScopedCrashKey On 2016/07/19 02:34:34, dcheng wrote: > On 2016/07/19 02:14:05, danakj wrote: > > The precompiler will replace the constructor defn with the macro otherwise? > > Yeah, this will garble the ctor. So this is a dumb hack to make it not shoot > itself in the foot. OK can you leave a quick comment on that. https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.h:46: enum ScopedCrashKeyNeedsNameTag { nit: I'd nest this inside ScopedCrashKey, beside (right below?) the nocompile constructor.
https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.cc (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.cc:16: #undef ScopedCrashKey On 2016/07/19 21:02:56, danakj wrote: > On 2016/07/19 02:34:34, dcheng wrote: > > On 2016/07/19 02:14:05, danakj wrote: > > > The precompiler will replace the constructor defn with the macro otherwise? > > > > Yeah, this will garble the ctor. So this is a dumb hack to make it not shoot > > itself in the foot. > > OK can you leave a quick comment on that. Done. https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... File base/debug/crash_logging.h (right): https://codereview.chromium.org/2137863002/diff/100001/base/debug/crash_loggi... base/debug/crash_logging.h:46: enum ScopedCrashKeyNeedsNameTag { On 2016/07/19 21:02:57, danakj wrote: > nit: I'd nest this inside ScopedCrashKey, beside (right below?) the nocompile > constructor. Done.
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2137863002/#ps120001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make it a compile error to use a crash key with no variable name. BUG= ========== to ========== Make it a compile error to use a crash key with no variable name. BUG= Committed: https://crrev.com/288f45d2de18e1668f03b2eee51fee8cd7ae2fad Cr-Commit-Position: refs/heads/master@{#407083} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/288f45d2de18e1668f03b2eee51fee8cd7ae2fad Cr-Commit-Position: refs/heads/master@{#407083} |
