[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Moving this to a top level discussion because it's easier to write replies here than in an inline comment.

>> If it's that the tool is trying to get the type information purely from the string representation, maybe the tool should be doing things differently - relying on Clang's AST/type APIs, rather than on inspecting the resulting string?
>
> I completely agree, however, the documentation on type API is not very good, and couldn't find a better way to get a string representation of the type using this API, besides manually traversing the AST and concatenating bits and pieces into a string which is essentially the same thing that TypePrinter does...

Sorry, I'm confused - it sounded like you didn't actually want a string, though - you want the type information to make various semantic-aware choices in your tool, yes?

> On the other hand, inventing new data structures to hold the type information or directly saving pointers to parts of the AST does not sound appealing to me, at least not for the current level of complexity I'm aiming for.

Generally the way to do it, if you want to introspect into the type, its template parameters, etc, then yes, keeping pointers to the AST or reparsing the name with Clang as-needed, would be the way to go - or walking the AST up-front and generating your own data structure (not necessarily a string) with the data you want in some structured format. Relying on the string printing of Clang to produce enough introspective information for a static analysis tool I think would be at odds with other tradeoffs that stringification is trying to achieve.

>> But if the type as a string is correct as far as C++ is concerned, is that not enough information (necessarily - the C++ compiler can determine all the type information from the type as written with only the top level types in the NTTP names)?
>
> I think that the essential problem here is the as far as C++ is concerned? What about the developer? Developers may have more benefit from getting a full type name in their diagnostic than just something that is correct as far as C++ is concerned.
>
> Consider this error message, which is completely useless and compare it to the much more informative errors by GCC and MSVC that immediately tell you what's wrong with your code.
>
> So, why shouldn't Clang behave the same way as well?

Again, I'm not advocating for the printing as-is, I think adding the top level name that disambiguates would be a good thing - and I think the GCC and MSVC examples somewhat show why adding all the other layers would be harmful to readability - there's a lot of text in those messages that doesn't directly help the user and gets in the way of identifying the important differences between the type names.

For instance, let's take the GCC message:

  <source>: In function 'void foo()':
  <source>:25:9: error: no match for 'operator=' (operand types are 'NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>' and 'NDArray<float, Height{DimensionImpl<Height>{Dimension{0}}}>')
     25 |     a = b;
        |         ^
  <source>:2:8: note: candidate: 'constexpr NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>& NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>::operator=(const NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>&)'
      2 | struct NDArray {};
        |        ^~~~~~~
  <source>:2:8: note:   no known conversion for argument 1 from 'NDArray<float, Height{DimensionImpl<Height>{Dimension{0}}}>' to 'const NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>&'
  <source>:2:8: note: candidate: 'constexpr NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>& NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>::operator=(NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>&&)'
  <source>:2:8: note:   no known conversion for argument 1 from 'NDArray<float, Height{DimensionImpl<Height>{Dimension{0}}}>' to 'NDArray<float, Width{DimensionImpl<Width>{Dimension{0}}}>&&'

And compare it to:

  <source>: In function 'void foo()':
  <source>:25:9: error: no match for 'operator=' (operand types are 'NDArray<float, Width{{{0}}}>' and 'NDArray<float, Height{{{0}}}>')
     25 |     a = b;
        |         ^
  <source>:2:8: note: candidate: 'constexpr NDArray<float, Width{{{0}}}>& NDArray<float, Width{{{0}}}>::operator=(const NDArray<float, Width{{{0}}}>&)'
      2 | struct NDArray {};
        |        ^~~~~~~
  <source>:2:8: note:   no known conversion for argument 1 from 'NDArray<float, Height{{{0}}}>' to 'const NDArray<float, Width{{{0}}}>&'
  <source>:2:8: note: candidate: 'constexpr NDArray<float, Width{{{0}}}>& NDArray<float, Width{{{0}}}>::operator=(NDArray<float, Width{{{0}}}>&&)'
  <source>:2:8: note:   no known conversion for argument 1 from 'NDArray<float, Height{{{0}}}>' to 'NDArray<float, Width{{{0}}}>&&'

That seems significantly easier to read/easier to identify the relevant difference for the developer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453