[PATCH] D134453: Disambiguate type names when printing NTTP types

Wed Oct 26 06:02:47 PDT 2022

aaron.ballman added a comment.

In D134453#3880794 <https://reviews.llvm.org/D134453#3880794>, @mizvekov wrote:

> In D134453#3880729 <https://reviews.llvm.org/D134453#3880729>, @dblaikie wrote:
>
>> I think we should restrict the discussion in this review to being only about the case where we have the canonical type (acknowledging that there's broader work to reduce the number of cases where that is what we have to deal with).
>
> I agree that we can leave that aside for this patch.

+1 :-)

>> When you say "for reasonably small objects" are you suggesting this patch/an nearish-term solution to the original problem presented here (`t1<{}>` with no info about the type of the NTTP) should include some heuristic to judge "reasonably small objects" and choose a different rendering in that case?
>
> I don't think we necessarily have to do something about that from the start, but it's probably a problem that is going to come up at some point.
> We are trying to render the whole structure of a class type in one line of diagnostic, seems like this breaks new ground.
>
>> OK, I'm not sure what to make of this, though - "the expression that initializes the NTTP" is something we don't have, right? Just the value, I think, at this point? So we're trying to pick some kind of rendering for that value - is it just the outer type and then braces and values (Using the "bad"/difficult case of nested subobjects - `t1{{3}, {4}}`) or with nested types (`t1{t2{3}, t3{4]}`) or with member names (`t1{.v1 = {.v2 = 3}, .v3 = {.v4 = 4}}`) or both (`t1{.v1 = t2{.v2 = 3}, .v3 = t3{.v4 = 4}}`).
>>
>> I guess you meant the first, `t1{{3}, {4}}`? OK, so I think you're agreeing with what I would prefer/outlined in https://reviews.llvm.org/D134453#3871251 (which isn't where this review was going when @aaron.ballman roped you in, it was going towards fully explicit (`t1{t2{3}, t3{4}}`))
>
> I am a bit torn actually, something like `t1{.name = "Bar"}` does look better ideally, but it's probably more prone to bad corner cases. Will probably not work well for tuples.

I also think this form will get chatty in practice. It's not that uncommon for real world code to have quite a few members in a structure, so I could see `t1{.name = "Bar"}` being reasonable while `t1{.name = "Bar", .age = 183, .height = -12, .weight = 1.2f, .eye_color = Eyes::Chartreuse, .smell = Smells::LikeTeenSpirit }` being a far less reasonable thing to print. That said, we could perhaps have a cut point so we print with the member names if there's < N of them and print without member names otherwise. But I'm not certain it's worth it.

> The second option, `t1{{3}, {4}}`, looks more well-rounded.

I can live with this approach if folks think it's the better way to go. But my concern still remains that the user has no idea what's being constructed by `{3}` or `{4}` without tracking down which constructor of `t1` is being called. Because of overloads, `std::initializer_list`, and conversion operators (and the fact that C++ initialization rules are a bit of a disaster), I think this can be unobvious sometimes without including type names. That said, after playing around a bit, I think those times might involve more contorted code than others are worried about, and so maybe it's not worth worrying about those cases. The fact that Clang is the only C++ compiler that doesn't print full type information makes me wary though. My thinking is that it's always more frustrating to have not enough information in a diagnostic than too much information (both are problems in their own ways though).

> The fully explicit `t1{t2{3}, t3{4}}` option seems like it would be both less readable and also even more prone to bad corner cases than the first (field names) option.

I can understand it being less readable for deep nesting situations, but I do not see why it would be prone to bad corner cases -- it's the most explicit form we can write (and matches the behavior of all other C++ compilers, from what we can tell).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453