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

Mon Oct 24 14:54:01 PDT 2022

dblaikie added a comment.

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

> In D134453#3880403 <https://reviews.llvm.org/D134453#3880403>, @dblaikie wrote:
>
>> Could you describe this in more detail? I find that to be more verbose than is necessary/helpful and probably the variable name based option to be more informative than the type name based option (if I have `Shape{3, 5}` (because I have two int members) that's less helpful than `Shape{.length = 3, .width = 5}` and all the type information seems pretty large/verbose.
>
> I agree printing all of the types of all of the subobjects is too much.

Ah, OK.

> I wouldn't object to what you are suggesting, it seems that if all we had was the canonical type of the NTTP, and no idea of what would be helpful for this diagnostic, then printing it in this designated-initializer style seems more readable, for reasonably small objects.

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

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?

> Though if we had the as-written type, I think the best option here is just printing the expression that initializes it, it's type, and perhaps if it was simple declref, pointing to the declaration, but that is probably situational.

Given this review's already gotten fairly muddled - hopefully we can defer the nuances of what to do when we have the type as-written for other threads/discussions that are addressing that case.

>> The former seems to not add a lot of value for the user - in this case due to the CRTP and other shenanigans. Admittedly other types might have nested aggregates that are more interesting/less obvious - but variable names still seem to me like they'd be more informative than types. (what I'm suggesting is the outermost has a type (since there's nothing else to use there, and C++ requires a type there) and nested things could use variable names (because they're more contextually relevant - differentiating a `width` and `height`, for instance, even though they might be the same type))
>
> Yeah, that definitely looks better than all the other options, if all we had was the NTTP value.
>
>> That feels liable to be /really/ verbose & make it harder to find the signal in all that noise, but maybe there's ways to make that work. Certainly having the type as written gives us more choices about how to do it/see what can be made to work, for sure.
>
> It seems to me that, at least for this diagnostic, and probably most others, just printing the expression that initializes the NTTP and it's type is enough.

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}}`))

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453