[PATCH] D76801: [AST] Print a<b<c>> without extra spaces in C++11 or later.

Fri Apr 17 13:32:31 PDT 2020

sammccall added a subscriber: dblaikie.
sammccall added a comment.

Sorry about the problems here, and thanks for letting me know...

In D76801#1989421 <https://reviews.llvm.org/D76801#1989421>, @rnk wrote:

> We updated the compiler and break some VS std::map visualizers:
>  https://crbug.com/1068394
>  I haven't 100% confirmed that this caused the issue, but looking at the code suggests that the names in our codeview output changed, which I would consider to be a regression. This should've been caught by clang's test suite. We already have codeview tests that try to ensure clang prints debug info names following MSVC's exact spacing, but apparently none broke, so this area was under-tested. I wonder if gdb or other DWARF consumers might be affected by this formatting change.

I've got a sinking feeling that I misunderstood the implications of updating CodeGenCXX/debug-info-template-explicit-specialization.cpp and the Modules/*DebugInfo.cpp tests.
I assumed that these were human-readable names, not things that tools rely on. As you say maybe the dwarf changes broke things.
I don't really know much about debug info, @dblaikie do you think the DWARF changes in those tests are safe? If not, should I be documenting/mitigating this or trying to undo it?

Regarding CodeView it sounds like we do need to undo this change. PrintingPolicy has:

  /// Use whitespace and punctuation like MSVC does. In particular, this prints
  /// anonymous namespaces as `anonymous namespace' and does not insert spaces
  /// after template arguments.
  unsigned MSVCFormatting : 1;

and this is set in `CGDebugInfo::getPrintingPolicy()` in CodeView mode.

That seems like the right knob to change this behavior.
I'm not sure I know enough to write the codeview test, but I'll give it a shot and send that to you.

> Two issues in the Chrome codebase where tests relied on the output of clang's diagnostics:
>  https://crbug.com/1064986
>  The compiler's diagnostic output isn't stable, so this is expected.

Yeah, obviously this changed a bunch of clang's own diagnostics tests too.
I'm not sure there's much to be done about this - even with perfect knowledge of such downstream tests, I don't think shying away from marginal improvements in diagnostics would be a great tradeoff.
Sorry about the churn though :-(

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76801