[libcxx-commits] [PATCH] D116938: [libc++][NFC] Use _LIBCPP_DEBUG_ASSERT in <list>

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM once my comment on lines 2007–2008 is addressed. Thanks, this is good cleanup!



================
Comment at: libcxx/include/list:1435-1436
 {
-#if _LIBCPP_DEBUG_LEVEL == 2
-    _LIBCPP_ASSERT(__get_const_db()->__find_c_from_i(_VSTD::addressof(__p)) == this,
-        "list::insert(iterator, x) called with an iterator not"
-        " referring to this list");
-#endif
+    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(_VSTD::addressof(__p)) == this,
+                         "list::insert(iterator, x) called with an iterator not referring to this list");
     __node_allocator& __na = base::__node_alloc();
----------------
I just wanted to call out that personally I dislike the old code's style of breaking a string literal randomly in the middle, because it means that if I get that error message and grep to find where it was generated — `git grep 'iterator not referring to this list'` — I would have gotten no results. Putting the message all on one line like you just did //is// the Right Way because it is greppable. :)


================
Comment at: libcxx/include/list:1973
 {
-    _LIBCPP_ASSERT(this != _VSTD::addressof(__c),
-                   "list::splice(iterator, list) called with this == &list");
-#if _LIBCPP_DEBUG_LEVEL == 2
-    _LIBCPP_ASSERT(__get_const_db()->__find_c_from_i(_VSTD::addressof(__p)) == this,
-        "list::splice(iterator, list) called with an iterator not"
-        " referring to this list");
-#endif
+    _LIBCPP_ASSERT(this != _VSTD::addressof(__c), "list::splice(iterator, list) called with this == &list");
+    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(_VSTD::addressof(__p)) == this,
----------------
Consider leaving this string message (all in one literal but) on the next line. I.e. don't change lines 2007–2008 at all, I think is what I'm saying, right?


================
Comment at: libcxx/include/list:2063
         for (const_iterator __i = __f; __i != __l; ++__i)
             _LIBCPP_ASSERT(__i != __p,
                            "list::splice(iterator, list, iterator, iterator)"
----------------
Consider `s/_LIBCPP_ASSERT/_LIBCPP_DEBUG_ASSERT/` on this line. IIUC, the change should have no effect, because this entire loop is guarded under `_LIBCPP_DEBUG_LEVEL == 2` already; but it would be more self-documenting — this is still a "debug-mode assertion", with all the connotations of `_LIBCPP_DEBUG_ASSERT` rather than whatever is connoted by a plain `_LIBCPP_ASSERT`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116938/new/

https://reviews.llvm.org/D116938