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

David Blaikie via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 26 14:52:09 PDT 2022


dblaikie added a comment.

>>> So we might print it with a syntax as if the class was a simple pod type, even though that might produce a completely different result if used in practice.
>>> I don't think we can do much better without having the actual expression used.
>>>
>>> That might be a point in favor of the designated-initializer style with the field names, that should never produce the wrong result if copy-pasted back to code.
>>
>> Oh, yeah, I hadn't considered that. :/ that is awkward & something I would certainly place some value in avoiding (was one of the issues I was motivated by in the earlier discussion, that it'd be useful if we produced an identifier that also happened to be valid code - but I wasn't considering the case where we might produce something valid-but-incorrect, just valid being better than invalid - but yeah, invalid being better than valid-but-incorrect).
>
> Yeah, I would place value in avoiding that outcome as well. Also, the point by @mizvekov that the types may be internal types is kind of compelling to me as well; I was thinking that the types involved are most often going to be ones users are aware of, but that's certainly not true for highly generic code like STL implementations.
>
> In the interests of getting this review unstuck, I think we should 1) remove the printing policy option (which should address the concerns from @dblaikie about use of that policy for introspection purposes); the policy is something that @DoDoENT can carry in their downstream, 2) keep the changes in TemplateBase.cpp and drop the changes from APValue.cpp.
>
> That gives us an incremental improvement over what the status quo is, and we can debate other improvements (if we can think of any we'd like to make) separately. Is everyone comfortable with this?

This would have the effect of  implementing (2) from here ( https://reviews.llvm.org/D134453#3869610 )? I'm good with that as an incremental improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453



More information about the cfe-commits mailing list