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

Tue Dec 27 10:02:24 PST 2022

Mordante added subscribers: erichkeane, ldionne, aaron.ballman.
Mordante added a comment.

In D139986#4017467 <https://reviews.llvm.org/D139986#4017467>, @dblaikie wrote:

> 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)

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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139986