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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
