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

Mon Oct 24 15:24:16 PDT 2022

mizvekov added a comment.

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.

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

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

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453