[clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

[Michael137]

Summary

This patch handles default integral non-type template parameters.

After this patch the clang TypePrinter will omit default integral
template arguments when the PrintingPolicy::SuppressDefaultTemplateArgs
option is specified and sets us up to be able to re-use
clang::isSubstitutedDefaultArgument from the DWARF CodeGen
component.

[dblaikie]

clang/test/CodeGenObjCXX/encode.mm

93–98

@aprantl any idea if this is good/OK? (I guess it probably is - but maybe these strings were never meant to ignore/suppress default arguments of any kind? or maybe this is an ABI sort of thing where it suppressing some but not others is now unchangeable?)

[Michael137]

• Simplify

[Michael137]

• Fix another test

[Michael137]

clang/test/CodeGenObjCXX/encode.mm

93–98

Good point. There was a thread on the cfe mailing list a while ago about the last time this broke: https://lists.llvm.org/pipermail/cfe-dev/2020-November/067194.html

This was @rsmith's stance:

I think some of the other recent TypePrinter changes might also risk
changing the @encode output. Generally it seems unwise for @encode to be
using the type pretty-printer if it wants to be ABI-stable; I don't think
it's reasonable to expect any guarantees as to the stability of
pretty-printed type names. I think USR generation suffers from similar
problems; it too uses the type pretty-printer to generate
supposedly-ABI-stable keys in at least some cases.

[Michael137]

clang/test/CodeGenObjCXX/encode.mm

93–98

see https://reviews.llvm.org/D90622

[aprantl]

clang/test/CodeGenObjCXX/encode.mm

93–98

To me it really looks like the intention of the feature is to not substitute default parameters. But if we stop doing this now it will likely result in a surprising code size increase, that may not be considered worth it compared to the risk of breaking ABI by changing a default template parameter.

As far as this patch is concerned, it's neutral to this decision (which may not have been a conscious one).
It's certainly not good that every type printer change is an ABI break.

[dblaikie]

Looks good to me

clang/test/CodeGenObjCXX/encode.mm

93–98

Awesome, thanks for tracking down that context @Michael137.

Not quite sure I'm following you @aprantl, but I think you're saying this change is OK/seems consistent with other changes?

[aprantl]

clang/test/CodeGenObjCXX/encode.mm

93–98

The current code (for @encode and USRs) seems to assume that TypePrinter output is stable.

I (personally) think that assumption ought to be wrong, because otherwise we'd never be able to make improvements such as this patch.

Aside from my personal preferences, practically this patch causes a problem for shipping an Objective-C compiler, since this is an ABI-breaking change. That's why I'd like to hear from someone with more insight into how @encoding is used in Objective-C wether this is something we need to be concerned about. If it is a concern we may need to add a TypePrinter configuration optimized for stability that preserves the current output format in eternity.

[aprantl]

clang/test/CodeGenObjCXX/encode.mm

93–98

To summarize an offline conversation about this: Because there are no system frameworks that vend Objective-C++ types we are not concerned by a potential ABI break caused by this patch.

LGTM!

[dblaikie]

clang/test/CodeGenObjCXX/encode.mm

93–98

Awesome - thanks for getting that info, @aprantl - any idea if we could write this down somewhere? (in comments in tests that verify @encoding if there aren't too many of them?) So it's easy/clear next time.

[Michael137]

• Rebase

[aprantl]

clang/test/CodeGenObjCXX/encode.mm

93–98

@Michael137 You could add a comment to this very test here, stating that the fact that the @encoding for C++ is effectively dependent on the TypePrinter implementation is a known bug.

[Michael137]

clang/test/CodeGenObjCXX/encode.mm

93–98

Done

[Michael137]

• Add comment re. ABI break

[Michael137]

• Fix commit message

[Michael137]

Missed couple of test cases in libcxx
About to fix those

[Mordante]

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.
Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

[dblaikie]

In D139986#4003873, @Mordante wrote:

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.
Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

Perhaps these LibC++ tests shouldn't be testing/using clang warnings? (could the warnings be turned off in the libc++ tests?)

[Mordante]

In D139986#4005997, @dblaikie wrote:

In D139986#4003873, @Mordante wrote:

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.
Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

Perhaps these LibC++ tests shouldn't be testing/using clang warnings? (could the warnings be turned off in the libc++ tests?)

In the failing tests we want to make sure we marked a struct as [[deprecated]]. In other tests we validate error messages to make sure things are ill-formed as mandated. We test the diagnostic to make sure the compilation fails for the expected reason and not a different compilation error. Do you have a suggestion how we can do that without checking Clang diagnostics?

In general I consider it bad practice to commit code when the CI is red.

[dblaikie]

In D139986#4008169, @Mordante wrote:

In D139986#4005997, @dblaikie wrote:

In D139986#4003873, @Mordante wrote:

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.

Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

Perhaps these LibC++ tests shouldn't be testing/using clang warnings? (could the warnings be turned off in the libc++ tests?)

In the failing tests we want to make sure we marked a struct as [[deprecated]].

At least looking at https://reviews.llvm.org/rG54d7c4dc870c5822df3d5ce538ad3936ac6405fe - that could be made more robust to clang changes by perhaps using // expected-warning {{deprecated}} - unlikely that the word deprecated will be removed from a deprecation warning, or that we'll emit a deprecation warning on some unrelated line from the usage. This would make the test more resilient to phrasing/spelling changes without, imho, significantly reducing the functionality of the test.

Though even more generally, I could see a test being written to use -Werror and various single uses of deprecated entities, checking that they're rejected - though that'd probably slow things down too much (singel process invocations for each deprecated entity) - so I'd say the above ({{deprecated}}) is probably a fairly good balance of making the tests less brittle while keeping them fast/functional.

(for locale.convenience for instance - I think that test could be simplified to name each type separately, rather than using them together - so they're on separate lines. Or maybe you can separate them onto different lines despite the nesting, splitting the outer templtae name over multiple lines)

In other tests we validate error messages to make sure things are ill-formed as mandated. We test the diagnostic to make sure the compilation fails for the expected reason and not a different compilation error. Do you have a suggestion how we can do that without checking Clang diagnostics?

In general I consider it bad practice to commit code when the CI is red.

My take on this is that, at least to my understanding, libc++ implemented this CI without discussiong/buy-in from other LLVM subprojects that libc++ depends on - that I'm not sure were/are willing to accept this extra constraint on their development. It's not suitable to bring up a CI in that way and then say "make sure you don't break this" - we didn't agree not to break it. Especially with tests like these that could/should be written differently to be less brittle. Perhaps I've misunderstood this & there is more explicit buy-in from Clang developers/owners about this relationship between the two subprojects?

Generally prior to this it was up to libc++ folks to cleanup after upstream project's breakage (like lldb still does, to some extent) - though buildbots, etc, do provide llvm/clang contributors some chance to proactively provide fixes.

[dblaikie]

Oh, and I meant to mention https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a doesn't seem, at least to my cursory reading, to show diagnostic checking failures, but crashes? Is there something broken in the UI that's meant to show the diagnostic mismatches you ended up fixing? (or is the link out of date at this point, and the original failures are no longer visible? or something else?)

[Mordante]

In D139986#4017467, @dblaikie wrote:

In D139986#4008169, @Mordante wrote:

In D139986#4005997, @dblaikie wrote:

In D139986#4003873, @Mordante wrote:

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.

Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

Perhaps these LibC++ tests shouldn't be testing/using clang warnings? (could the warnings be turned off in the libc++ tests?)

In the failing tests we want to make sure we marked a struct as [[deprecated]].

At least looking at https://reviews.llvm.org/rG54d7c4dc870c5822df3d5ce538ad3936ac6405fe - that could be made more robust to clang changes by perhaps using // expected-warning {{deprecated}} - unlikely that the word deprecated will be removed from a deprecation warning, or that we'll emit a deprecation warning on some unrelated line from the usage. This would make the test more resilient to phrasing/spelling changes without, imho, significantly reducing the functionality of the test.

Though even more generally, I could see a test being written to use -Werror and various single uses of deprecated entities, checking that they're rejected - though that'd probably slow things down too much (singel process invocations for each deprecated entity) - so I'd say the above ({{deprecated}}) is probably a fairly good balance of making the tests less brittle while keeping them fast/functional.

(for locale.convenience for instance - I think that test could be simplified to name each type separately, rather than using them together - so they're on separate lines. Or maybe you can separate them onto different lines despite the nesting, splitting the outer templtae name over multiple lines)

I don't see how that helps to validate whether we have the expected error or an error due to ill-formed code. For example just forgetting a semicolon.

In other tests we validate error messages to make sure things are ill-formed as mandated. We test the diagnostic to make sure the compilation fails for the expected reason and not a different compilation error. Do you have a suggestion how we can do that without checking Clang diagnostics?

In general I consider it bad practice to commit code when the CI is red.

My take on this is that, at least to my understanding, libc++ implemented this CI without discussiong/buy-in from other LLVM subprojects that libc++ depends on - that I'm not sure were/are willing to accept this extra constraint on their development. It's not suitable to bring up a CI in that way and then say "make sure you don't break this" - we didn't agree not to break it. Especially with tests like these that could/should be written differently to be less brittle. Perhaps I've misunderstood this & there is more explicit buy-in from Clang developers/owners about this relationship between the two subprojects?

Generally prior to this it was up to libc++ folks to cleanup after upstream project's breakage (like lldb still does, to some extent) - though buildbots, etc, do provide llvm/clang contributors some chance to proactively provide fixes.

This CI has been discussed with members of the Clang community. In the past I've spoken with @aaron.ballman. I know @erichkeane requested @ldionne to create this CI recently. (I wasn't present during this meeting.) I don't know whether or not the creation of this CI has been further communicated in the Clang community. Note that this CI is a benefit for Clang too. Some patches that break libc++, also break code in other external projects. This patch probably doesn't break external projects, but @erichkeane's D126907 was reverted several times due to breakage in external projects. I also discovered some issues with that patch in libc++, so there having libc++ as test codebase would have been helpful.

I'm not asking Clang developers to fix libc++. But I ask not to commit code which is known to break other subprojects without giving that subproject a headsup.

In D139986#4017468, @dblaikie wrote:

Oh, and I meant to mention https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a doesn't seem, at least to my cursory reading, to show diagnostic checking failures, but crashes? Is there something broken in the UI that's meant to show the diagnostic mismatches you ended up fixing? (or is the link out of date at this point, and the original failures are no longer visible? or something else?)

When I look at that output for C++2b I see

error: 'warning' diagnostics expected but not seen: 

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t, 1114111, 0>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t, 1114111, 0>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t, 1114111, 0>' is deprecated

error: 'warning' diagnostics seen but not expected: 

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t>' is deprecated

For the modular build I also see ICEs, which are compiler bugs.

[dblaikie]

In D139986#4017650, @Mordante wrote:

In D139986#4017467, @dblaikie wrote:

In D139986#4008169, @Mordante wrote:

In D139986#4005997, @dblaikie wrote:

In D139986#4003873, @Mordante wrote:

In D139986#4001180, @Michael137 wrote:

Missed couple of test cases in libcxx
About to fix those

There were more breakage due to this patch, which I fixed in D140272.

Next time please don't commit patches when the pre-commit CI is red. This build https://buildkite.com/llvm-project/libcxx-ci/builds/16118 shows the patch will break libc++. These pre-commit CI jobs were added specifically to aid the Clang developers to validate whether their changes break libc++. Libc++ heavily relies on its pre-commit CI so breaking the CI has a huge impact.

Note that when libc++ breaks there might be other projects that use the latest Clang HEAD that will be affected too. (Not likely with diagnostics, but likely when the modular build fails.)

When you have issues resolving the libc++ issues you can always reach out to us for assistance.

Perhaps these LibC++ tests shouldn't be testing/using clang warnings? (could the warnings be turned off in the libc++ tests?)

In the failing tests we want to make sure we marked a struct as [[deprecated]].

At least looking at https://reviews.llvm.org/rG54d7c4dc870c5822df3d5ce538ad3936ac6405fe - that could be made more robust to clang changes by perhaps using // expected-warning {{deprecated}} - unlikely that the word deprecated will be removed from a deprecation warning, or that we'll emit a deprecation warning on some unrelated line from the usage. This would make the test more resilient to phrasing/spelling changes without, imho, significantly reducing the functionality of the test.

Though even more generally, I could see a test being written to use -Werror and various single uses of deprecated entities, checking that they're rejected - though that'd probably slow things down too much (singel process invocations for each deprecated entity) - so I'd say the above ({{deprecated}}) is probably a fairly good balance of making the tests less brittle while keeping them fast/functional.

(for locale.convenience for instance - I think that test could be simplified to name each type separately, rather than using them together - so they're on separate lines. Or maybe you can separate them onto different lines despite the nesting, splitting the outer templtae name over multiple lines)

I don't see how that helps to validate whether we have the expected error or an error due to ill-formed code. For example just forgetting a semicolon.

reducing the wording checked for wouldn't have that problem - but yeah, going to totally wording agnostic testing wouldbe trickier. It could still avoid the "missing semicolon" problem by compiling with/without -Wdeprecated - expecting success without it, and failure with it. But I suspect the process launching overhead wouldn't be ideal for that direction anyway. At least reducing the dependence on specific warning wording/spelling would be help make the testing less brittle.

In other tests we validate error messages to make sure things are ill-formed as mandated. We test the diagnostic to make sure the compilation fails for the expected reason and not a different compilation error. Do you have a suggestion how we can do that without checking Clang diagnostics?

In general I consider it bad practice to commit code when the CI is red.

My take on this is that, at least to my understanding, libc++ implemented this CI without discussiong/buy-in from other LLVM subprojects that libc++ depends on - that I'm not sure were/are willing to accept this extra constraint on their development. It's not suitable to bring up a CI in that way and then say "make sure you don't break this" - we didn't agree not to break it. Especially with tests like these that could/should be written differently to be less brittle. Perhaps I've misunderstood this & there is more explicit buy-in from Clang developers/owners about this relationship between the two subprojects?

Generally prior to this it was up to libc++ folks to cleanup after upstream project's breakage (like lldb still does, to some extent) - though buildbots, etc, do provide llvm/clang contributors some chance to proactively provide fixes.

This CI has been discussed with members of the Clang community. In the past I've spoken with @aaron.ballman. I know @erichkeane requested @ldionne to create this CI recently. (I wasn't present during this meeting.) I don't know whether or not the creation of this CI has been further communicated in the Clang community. Note that this CI is a benefit for Clang too. Some patches that break libc++, also break code in other external projects. This patch probably doesn't break external projects, but @erichkeane's D126907 was reverted several times due to breakage in external projects. I also discovered some issues with that patch in libc++, so there having libc++ as test codebase would have been helpful.

*nod* it still seems a bit unreliable, though (like doesn't report existing failures differently from new failures, maybe? Producing false positive failures), which produces some tensions.

I'm not asking Clang developers to fix libc++. But I ask not to commit code which is known to break other subprojects without giving that subproject a headsup.

Ah, perhaps there's some way to automate that then? So it doesn't involve humans - the precommit CI infrastructure could email libc++ developers about the failure directly, rather than requiring the human developer to be the message carrier?

In D139986#4017468, @dblaikie wrote:

Oh, and I meant to mention https://buildkite.com/llvm-project/libcxx-ci/builds/16118#01851abb-f13e-4977-91df-b4274402b65a doesn't seem, at least to my cursory reading, to show diagnostic checking failures, but crashes? Is there something broken in the UI that's meant to show the diagnostic mismatches you ended up fixing? (or is the link out of date at this point, and the original failures are no longer visible? or something else?)

When I look at that output for C++2b I see

error: 'warning' diagnostics expected but not seen: 

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t, 1114111, 0>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t, 1114111, 0>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t, 1114111, 0>' is deprecated

error: 'warning' diagnostics seen but not expected: 

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 19: 'codecvt_utf8<wchar_t>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 20: 'codecvt_utf16<wchar_t>' is deprecated

  File /home/libcxx-builder/.buildkite-agent/builds/61f52b66aa77-1/llvm-project/libcxx-ci/libcxx/test/std/localization/locale.stdcvt/depr.verify.cpp Line 21: 'codecvt_utf8_utf16<wchar_t>' is deprecated

For the modular build I also see ICEs, which are compiler bugs.

The modular build ICE is something pre-existing/not caused by this patch, right? Makes it hard to know when there are real failures if there are false positives like this.

And at least for me, when I go to that link and click on the "C++2b" bar I only see the modular crash, I don't see these deprecated diagnostic errors:

https://imgur.com/a/ZJdbumR

And looking at the raw log ah, there I do see the failures if I search for them. But amongst so much other noise that I don't think it's reasonable to expect anyone to find them, really? As they're all of about 50 lines in a many thousand line log file.

[Mordante]

I think it would be better to have a meeting to discuss this further instead of posting in this review. I propose to do it after everybody's back from their holidays. WDYT?

[ldionne]

@dblaikie

We added the libc++ tests to the Clang pre-commit CI after discussing with @erichkeane since he told me breaking libc++ was a recurring pain point, and having a way to detect that would be greatly appreciated by the Clang folks. The goal was really only to help Clang developers catch more issues earlier. I believe this confusion is only the result of miscommunication within the Clang community -- it seems that not all Clang developers know equally well what tools are available to them and what each bit of infrastructure should be used for. @erichkeane @aaron.ballman Would it make sense for you folks to post something on Discourse to explain what the expectations are w.r.t. Clang pre-commit CI?

[erichkeane]

In D139986#4040185, @ldionne wrote:

@dblaikie

We added the libc++ tests to the Clang pre-commit CI after discussing with @erichkeane since he told me breaking libc++ was a recurring pain point, and having a way to detect that would be greatly appreciated by the Clang folks. The goal was really only to help Clang developers catch more issues earlier. I believe this confusion is only the result of miscommunication within the Clang community -- it seems that not all Clang developers know equally well what tools are available to them and what each bit of infrastructure should be used for. @erichkeane @aaron.ballman Would it make sense for you folks to post something on Discourse to explain what the expectations are w.r.t. Clang pre-commit CI?

That is DEFINITELY true, the Clang pre-commit CI is historically really unreliable, so seeing a failed CI is something that even the most experienced of us are ignoring quite a bit thanks to its unreliability. An announcement of some sort that we CAN trust the libc++ one is perhaps a good idea. Perhaps I'll work with Aaron to come up with a message.

[dblaikie]

In D139986#4040208, @erichkeane wrote:

In D139986#4040185, @ldionne wrote:

@dblaikie

We added the libc++ tests to the Clang pre-commit CI after discussing with @erichkeane since he told me breaking libc++ was a recurring pain point, and having a way to detect that would be greatly appreciated by the Clang folks. The goal was really only to help Clang developers catch more issues earlier. I believe this confusion is only the result of miscommunication within the Clang community -- it seems that not all Clang developers know equally well what tools are available to them and what each bit of infrastructure should be used for. @erichkeane @aaron.ballman Would it make sense for you folks to post something on Discourse to explain what the expectations are w.r.t. Clang pre-commit CI?

That is DEFINITELY true, the Clang pre-commit CI is historically really unreliable, so seeing a failed CI is something that even the most experienced of us are ignoring quite a bit thanks to its unreliability. An announcement of some sort that we CAN trust the libc++ one is perhaps a good idea. Perhaps I'll work with Aaron to come up with a message.

FWIW, my experience with the libc++ one hasn't been great either. Both in terms of test quality, like the diagnostic issue in this case (changing the phrasing of a diagnostic doesn't seem like it should cause libc++ test failures - I think these tests should be rephrased to be less brittle) and in terms of ease of use (I still don't know how I/others should've been able to reliably/easily get to the failure from the links in this review - all I could see were modular crash dumps).

clang/lib/AST/TypePrinter.cpp

if (Pattern.getKind() == TemplateArgument::Expression) {
     if (auto *DRE =
             dyn_cast<DeclRefExpr>(Pattern.getAsExpr()->IgnoreParenImpCasts())) {
       if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(DRE->getDecl()))
         return NTTP->getDepth() == Depth && Args.size() > NTTP->getIndex() &&
                Args[NTTP->getIndex()].structurallyEquals(Arg);
     }
   }
 
+  if (Arg.getKind() == TemplateArgument::Integral &&
+      Pattern.getKind() == TemplateArgument::Expression) {
+    Expr const *expr = Pattern.getAsExpr();
+
+    if (!expr->isValueDependent() && expr->isIntegerConstantExpr(Ctx)) {
+      return llvm::APSInt::isSameValue(expr->EvaluateKnownConstInt(Ctx),
+                                       Arg.getAsIntegral());
+    }
+  }
+
   if (Arg.getKind() != Pattern.getKind())
     return false;
 
   if (Arg.getKind() == TemplateArgument::Type)
     return isSubstitutedType(Ctx, Arg.getAsType(), Pattern.getAsType(), Args,
                              Depth);
 
   if (Arg.getKind() == TemplateArgument::Template) {

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-0x.cpp

 template <int N> struct C; 
 template <class T1, int N> struct D; 
 template <class T1, class T2, int N = 17> struct E;
 
 eval<A<int>> eA;
 eval<B<int, float>> eB;
 eval<C<17>> eC; // expected-error{{implicit instantiation of undefined template 'eval<C<17>>'}}
 eval<D<int, 17>> eD; // expected-error{{implicit instantiation of undefined template 'eval<D<int, 17>>'}}
-eval<E<int, float>> eE; // expected-error{{implicit instantiation of undefined template 'eval<E<int, float, 17>>}}
+eval<E<int, float>> eE; // expected-error{{implicit instantiation of undefined template 'eval<E<int, float>>}}
 
 template<template <int ...N> class TT> struct X0 { }; // expected-note{{previous non-type template parameter with type 'int' is here}}
 template<int I, int J, int ...Rest> struct X0a;
 template<int ...Rest> struct X0b;
 template<int I, long J> struct X0c; // expected-note{{template non-type parameter has a different type 'long' in template argument}}
 
 X0<X0a> inst_x0a;
 X0<X0b> inst_x0b;

clang/test/CodeGenObjCXX/encode.mm

namespace rdar9357400 {
   
   template<typename Element, int Size>
   class vector< Element, fixed<Size> >
   : public fixed_1D<Element,Size> { };
 
   typedef vector< float,  fixed<4> > vector4f;
 
   // FIXME: This difference is due to D76801. It was probably an unintentional change. Maybe we want to undo it?
-  // CHECKCXX98: @_ZN11rdar93574002ggE ={{.*}} constant [49 x i8] c"{vector<float, rdar9357400::fixed<4, -1> >=[4f]}\00"
-  // CHECKCXX20: @_ZN11rdar93574002ggE ={{.*}} constant [48 x i8] c"{vector<float, rdar9357400::fixed<4, -1>>=[4f]}\00"
+  // @encoding for C++ is dependent on the TypePrinter implementation, which is a known issue. But since there
+  // are currently no system frameworks that vend Objective-C++ types, a potential ABI break caused by changes
+  // to the TypePrinter should not be a concern.
+  // CHECKCXX98: @_ZN11rdar93574002ggE ={{.*}} constant [45 x i8] c"{vector<float, rdar9357400::fixed<4> >=[4f]}\00"
+  // CHECKCXX20: @_ZN11rdar93574002ggE ={{.*}} constant [44 x i8] c"{vector<float, rdar9357400::fixed<4>>=[4f]}\00"
   extern const char gg[] = @encode(vector4f);
 }
 
 // rdar://9624314
 namespace rdar9624314 {
   struct B2 { int x; };
   struct B3 {};
   struct S : B2, B3 {};
 

clang/test/Misc/diag-template-diffing.cpp

 A<1> a1 = A<>();
 A<> a2 = A<1>();
 B<> b1 = B<int>();
 B<int> b2 = B<>();
 B<> b3 = B<const A<>>();
 B<const A<>> b4 = B<>();
 // CHECK-ELIDE-NOTREE: error: no viable conversion from 'A<(default) 0>' to 'A<1>'
 // CHECK-ELIDE-NOTREE: error: no viable conversion from 'A<1>' to 'A<(default) 0>'
-// CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<int>' to 'B<(default) ZeroArgs::A<0>>'
-// CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<(default) ZeroArgs::A<0>>' to 'B<int>'
+// CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<int>' to 'B<(default) ZeroArgs::A<>>'
+// CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<(default) ZeroArgs::A<>>' to 'B<int>'
 // CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<const A<...>>' to 'B<A<...>>'
 // CHECK-ELIDE-NOTREE: error: no viable conversion from 'B<A<...>>' to 'B<const A<...>>'
 }
 
 namespace TypeAlias {
 
 template <typename T> class vector {};
 

clang/test/Misc/diag-template.cpp

namespace default_args {
   };
   template<class T, class Deleter> class unique_ptr<T[], Deleter> {};
   void test2() {
     unique_ptr<string> ups;
     f(ups).reset(); // expected-error {{no member named 'reset' in 'default_args::unique_ptr<default_args::basic_string<char>>'}}
     f(ups).f(); // expected-note {{in instantiation of member function 'default_args::unique_ptr<default_args::basic_string<char>>::f' requested here}}
   }
 
-  template<int A, int B = A> struct Z { int error[B]; }; // expected-error {{negative size}}
+  template<int A, int B = A, int C = 42> struct Z { int error[B]; }; // expected-error {{negative size}}
   Z<-1> z; // expected-note {{in instantiation of template class 'default_args::Z<-1>' requested here}}
 
   template<template<typename> class A = allocator, template<typename> class B = A> struct Q {};
   void test3() {
     f(Q<>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<>'}}
     f(Q<allocator>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<>'}}
     f(Q<allocator, allocator>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<>'}}
     f(Q<char_traits>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<default_args::char_traits>'}}
     f(Q<char_traits, char_traits>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<default_args::char_traits>'}}
     f(Q<char_traits, allocator>()).g(); // expected-error {{no member named 'g' in 'default_args::Q<default_args::char_traits, default_args::allocator>'}}
   }
 }

clang/test/SemaCUDA/device-use-host-var.cu

struct  not_a_texture {
   }
 };
 
 template<>
 not_a_texture<int> not_a_texture<int>::ref; // dev-note {{host variable declared here}}
 
 __device__ void test_not_a_texture() {
   not_a_texture<int> inst;
-  inst.c(); // dev-note {{in instantiation of member function 'not_a_texture<int, 1, 1>::c' requested here}}
+  inst.c(); // dev-note {{in instantiation of member function 'not_a_texture<int>::c' requested here}}
 }
 
 // Test static variable in host function used by device function.
 void test_static_var_host() {
   for (int i = 0; i < 10; i++) {
     static int x; // dev-note {{host variable declared here}}
     struct A {
       __device__ int f() {

clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp

 };
 } // namespace NS
 using NS::ForLoopAwaiterCoawaitLookup;
 
 template <class T>
 ForLoopAwaiterCoawaitLookup test_coawait_lookup(T) {
   Range<T> R;
   for co_await (auto i : R) {}
-  // expected-error@-1 {{no member named 'await_ready' in 'CoawaitTag<Iter<int>, false>'}}
+  // expected-error@-1 {{no member named 'await_ready' in 'CoawaitTag<Iter<int>>'}}
 }
 template ForLoopAwaiterCoawaitLookup test_coawait_lookup(int); // expected-note {{requested here}}
 
 // FIXME: This test should fail as well since the newly declared operator co_await
 // should not be found by lookup.
 namespace NS2 {
 template <class Iter>
 Awaiter<Iter> operator co_await(CoawaitTag<Iter, false>);
 }
 using NS2::operator co_await;
 template ForLoopAwaiterCoawaitLookup test_coawait_lookup(long);

clang/test/SemaCXX/co_await-range-for.cpp

 };
 } // namespace NS
 using NS::ForLoopAwaiterCoawaitLookup;
 
 template <class T>
 ForLoopAwaiterCoawaitLookup test_coawait_lookup(T) {
   Range<T> R;
   for co_await(auto i : R) {} // expected-warning {{'for co_await' belongs to CoroutineTS instead of C++20, which is deprecated}}
-  // expected-error@-1 {{no member named 'await_ready' in 'CoawaitTag<Iter<int>, false>'}}
+  // expected-error@-1 {{no member named 'await_ready' in 'CoawaitTag<Iter<int>>'}}
 }
 template ForLoopAwaiterCoawaitLookup test_coawait_lookup(int); // expected-note {{requested here}}
 
 // FIXME: This test should fail as well since the newly declared operator co_await
 // should not be found by lookup.
 namespace NS2 {
 template <class Iter>
 Awaiter<Iter> operator co_await(CoawaitTag<Iter, false>);
 }
 using NS2::operator co_await;
 template ForLoopAwaiterCoawaitLookup test_coawait_lookup(long);

clang/test/SemaCXX/coroutines-exp-namespace.cpp

 }
 
 template <int ID = 0>
 struct NoCopy {
   NoCopy(NoCopy const &) = delete; // expected-note 2 {{deleted here}}
 };
 template <class T, class U>
 void test_dependent_param(T t, U) {
-  // expected-error@-1 {{call to deleted constructor of 'NoCopy<0>'}}
+  // expected-error@-1 {{call to deleted constructor of 'NoCopy<>'}}
   // expected-error@-2 {{call to deleted constructor of 'NoCopy<1>'}}
   ((void)t);
   co_return 42;
 }
 template void test_dependent_param(NoCopy<0>, NoCopy<1>); // expected-note {{requested here}}
 
 namespace CoroHandleMemberFunctionTest {
 struct CoroMemberTag {};

clang/test/SemaCXX/coroutines.cpp

 }
 
 template <int ID = 0>
 struct NoCopy {
   NoCopy(NoCopy const&) = delete; // expected-note 2 {{deleted here}}
 };
 template <class T, class U>
 void test_dependent_param(T t, U) {
-  // expected-error@-1 {{call to deleted constructor of 'NoCopy<0>'}}
+  // expected-error@-1 {{call to deleted constructor of 'NoCopy<>'}}
   // expected-error@-2 {{call to deleted constructor of 'NoCopy<1>'}}
   ((void)t);
   co_return 42;
 }
 template void test_dependent_param(NoCopy<0>, NoCopy<1>); // expected-note {{requested here}}
 
 namespace CoroHandleMemberFunctionTest {
 struct CoroMemberTag {};

clang/test/SemaCXX/cxx11-call-to-deleted-constructor.cpp

public:
     Matrix(const std::initializer_list<value_type>& list) = delete; // expected-note {{'Matrix' has been explicitly marked deleted here}}
 
 };
 
 void getLaplacianClosedForm()
 {
     Matrix<double> winI(0, 3);
     RGBFValue* inputPreL;
-    winI = { inputPreL->at() }; // expected-error {{call to deleted constructor of 'cva::Matrix<double, 0, 0> &&'}}
+    winI = { inputPreL->at() }; // expected-error {{call to deleted constructor of 'cva::Matrix<double> &&'}}
 }
 
 }

clang/test/SemaTemplate/deduction-guide.cpp

 // CHECK: |   `-ConstantExpr {{.*}} 'char'
 // CHECK: |     |-value: Int 120
 // CHECK: |     `-CharacterLiteral {{.*}} 'char' 120
 // CHECK: |-TemplateTypeParmDecl {{.*}} typename depth 0 index 1 U
 // CHECK: |-ParenExpr {{.*}} 'bool'
 // CHECK: | `-CXXBoolLiteralExpr {{.*}} 'bool' false
 // CHECK: |-CXXDeductionGuideDecl {{.*}} implicit <deduction guide for F> 'auto (type-parameter-0-1) -> F<>'
 // CHECK: | `-ParmVarDecl {{.*}} 'type-parameter-0-1'
-// CHECK: `-CXXDeductionGuideDecl {{.*}} implicit <deduction guide for F> 'auto (int) -> F<'x'>'
+// CHECK: `-CXXDeductionGuideDecl {{.*}} implicit <deduction guide for F> 'auto (int) -> F<>'
 // CHECK:   |-TemplateArgument integral 120
 // CHECK:   |-TemplateArgument type 'int'
 // CHECK:   | `-BuiltinType {{.*}} 'int'
 // CHECK:   `-ParmVarDecl {{.*}} 'int':'int'
 // CHECK: FunctionProtoType {{.*}} 'auto (type-parameter-0-1) -> F<>' dependent trailing_return cdecl
 // CHECK: |-InjectedClassNameType {{.*}} 'F<>' dependent
 // CHECK: | `-CXXRecord {{.*}} 'F'
 // CHECK: `-TemplateTypeParmType {{.*}} 'type-parameter-0-1' dependent depth 0 index 1

clang/test/SemaTemplate/dependent-names.cpp

 
 namespace PR11421 {
 template < unsigned > struct X {
   static const unsigned dimension = 3;
   template<unsigned dim=dimension> 
   struct Y: Y<dim> { }; // expected-error{{circular inheritance between 'Y<dim>' and 'Y<dim>'}}
 };
 typedef X<3> X3;
-X3::Y<>::iterator it; // expected-error {{no type named 'iterator' in 'PR11421::X<3>::Y<3>'}}
+X3::Y<>::iterator it; // expected-error {{no type named 'iterator' in 'PR11421::X<3>::Y<>'}}
 }
 
 namespace rdar12629723 {
   template<class T>
   struct X {
     struct C : public C { }; // expected-error{{circular inheritance between 'C' and 'rdar12629723::X::C'}}
 
     struct B;