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

[David Blaikie via Phabricator via libcxx-commits]

dblaikie 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.

Yeah, for sure - I've been meaning to gather more of the size data too, but that's a bit more involved.

A rough explanation, based on what I can test:

1. for alias templates both LLVM and GCC's debug info is a bit broken-ish. GCC produces "simplified" template names (not including any template parameters in the names) and Clang's currently producing duplicate alias template definitions depending on the spelling of the parameter (eg: `conditional_t<false, A, B>` is a distinct bit of debug info from `conditional_t<0, A, B>` - fixing this I guess will significantly reduce debug info size, they should probably both canonicalize to `conditional_t<false, A, B>`).

2. gdb at least looks through/appears to ignore the typedefs entirely (if you `ptype x` where x is of some conditional_t type, it shows the underlying type (eg: `A` or `B` in the examples above), no mention of conditional)

3. Even if a debugger does render the `conditional_t` specialization (lldb does in some cases - "member reference base type 'conditional_t<true, double, float>' (aka 'double') is not a structure or union") once (1) is fixed, you only get the canonical representation of the type, so you don't get a lot of information - like the expression that was used for condition.

Debuggers could benefit from other typedefs - like knowing that some `int*` is actually a `vector<int>::iterator` might be helpful for a user - but type traits I don't think provide the same sort of value (eg: "remove_const<const T> (aka ("T"))" doesn't add a lot of value for the developer, etc) - which was the conclusion we reached/the basis on which the plumbing (& the uses that are already in-tree) for this was added previously.



================
Comment at: libcxx/include/__type_traits/conditional.h:39-45
+struct _LIBCPP_TEMPLATE_VIS conditional {
+  _LIBCPP_NODEBUG typedef _If type;
+};
 template <class _If, class _Then>
-    struct _LIBCPP_TEMPLATE_VIS conditional<false, _If, _Then> {typedef _Then type;};
+struct _LIBCPP_TEMPLATE_VIS conditional<false, _If, _Then> {
+  _LIBCPP_NODEBUG typedef _Then type;
+};
----------------
ldionne wrote:
> 
Should be do the typedef->using as a separate change? Happy to send another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131082