[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D139986#4008169 <https://reviews.llvm.org/D139986#4008169>, @Mordante wrote:

> In D139986#4005997 <https://reviews.llvm.org/D139986#4005997>, @dblaikie wrote:
>
>> In D139986#4003873 <https://reviews.llvm.org/D139986#4003873>, @Mordante wrote:
>>
>>> In D139986#4001180 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139986