[libcxx-commits] [PATCH] D131082: [libcxx] Add _LIBCPP_NODEBUG to std::conditional related typedfs

[Eric Fiselier via Phabricator via libcxx-commits]

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

In D131082#3792754 <https://reviews.llvm.org/D131082#3792754>, @ldionne wrote:

> Could someone share something like a screenshot of the debugging experience before and after? I'm also kind of uneasy about removing debug information, but I think it might be because I don't properly understand what effect it has on the debugging experience. @dblaikie you have a lot more experience with this than I do, so I assume I'd share your opinion if I saw what it looked like.
>
> Requesting changes so I'm notified of any update.

Let me try and convince you by way of analogy. The question, as I see it, is whether for a computed value `v`, we want the debugger to show us the expression that computed that value, or the value itself. Let's consider this question for a normal, non-metaprogramming, conditional expresssion. For example

  void foo(int x) {
    // BREAKPOINT
  }
  int main(int argc, char** argv) {
    foo(argc == 0 ? 42 : 101);
  }

Now imagine I'm debugging the call to `foo` and I want to know the value of `x`, so I ask the debugger to print the variable and I get

  (lldb) var
  > (int) x = 101

But imagine if instead the debugger printed the expressions used to compute `x`. I.e.

  (lldb) var
  `(int) x = false ? 42 : 101`

I already know the expression used to produce `y` from reading the source, and asking me to compute the value myself from intermediate expressions is not helpful. And as the source expressions get more complex, the output from the debugger becomes less and less useful.

Unfortunately the latter option is what we're providing to users since we don't apply `_LIBCPP_NODEBUG` to conditional.

Here's a concrete (but silly) example:

  debug.cpp
  
  // forward the value as is, unless it's a c-string, then forward as a string_view.
  template <class T>
  auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
  // BREAKPOINT
  // (lldb) var
   return v;
  }
  
  int main() {
    my_forward<int>(42);
  }

Currently, when I step to the breakpoint and print variables at the breakpoint, I get

  * thread #1, name = 'debug.out', stop reason = step in
      frame #0: 0x0000555555555197 debug.out`auto my_forward<int>(v=42) at debug.cpp:10:10
     7   	// forward the value as is, unless it's a c-string, then forward as a string_view.
     8   	template <class T>
     9   	auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
  -> 10  	  return v;
      	         ^
     11  	}
     12  	
     13  	int main(int argc, char** argv) {
  ...
  (lldb) var
  (std::conditional<false, std::basic_string_view<char, std::char_traits<char> >, int>::type) v = 42

However, with this change, the debugger prints the following

  * thread #1, name = 'debug.out', stop reason = step in
      frame #0: 0x0000555555555197 debug.out`auto my_forward<int>(v=42) at debug.cpp:10:10
     7   	// forward the value as is, unless it's a c-string, then forward as a string_view.
     8   	template <class T>
     9   	auto my_forward(typename std::conditional<std::is_same<T, const char*>::value, std::string_view, T>::type v) {
  -> 10  	  return v;
      	         ^
     11  	}
  
  (lldb) var
  (int) v = 42

Notice that in both cases, the debugger prints the source code, providing enough context for the reader.

I believe this change not only significatly improves debug binary bloating, but also improves the debugging experience for our users.
And that we should apply `_LIBCPP_NODEBUG` aggressively to libc++'s typedefs, including inside conditional.

@ldionne Does this help answer your questions.

This change LGTM, but I'll let @ldionne stamp it for libc++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131082