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

Wed Dec 28 06:20:33 PST 2022

dblaikie added a comment.

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

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

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

*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 <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.

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 <https://buildkite-cloud.s3.amazonaws.com/logs-by-pipeline/3f135a9c-106c-4a9e-a748-96d79d4149c3/01851abb-10d2-45ff-b442-5760a020df68/01851abb-f13e-4977-91df-b4274402b65a.log?response-content-disposition=inline&response-content-type=text%2Fplain&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAQPCP3C7LQG3TTADG%2F20221228%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20221228T141745Z&X-Amz-Expires=600&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEPr%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLWVhc3QtMSJHMEUCIQCzshO1Yu8j%2BU37DDwBeT%2FTo3mOMcMk0Yt%2FPyNDz8fmswIgIGr%2BfNhLi64pvo6hhRqPmDd4Bj8AcFEm4O2fbPXAdy8qzAQIQxAAGgwwMzIzNzk3MDUzMDMiDLCte2%2B4I9d40AN7kiqpBNNLj4TZamjuYd07H1pu1VyBSckiIccc0NOi8XEBSyoHPfUGVLof5bHGmkumO6rg4D%2FHCfIk3W2z4viftW29tLEjGGIbMw5h9jvNUepPOqUPLzCjNshuESn6nutL4w9vUxRL8zNpwNHsr%2B%2FtbGaZbwev6d55Hi%2F7PHTVwVusKp60el5zFlI3VAVB2Pnc6sax3AQsf1TOWcMcDIArlCRAkvjecAlpT%2BUVTbzKG1ai0KwiyZbrThurkI3qySr0%2Fn8f5Vkli8%2F4vFsuQTV6L586b%2F7gnw7w7Am4zw2ieqD5UZTXAY3PdR1ZPzbZ6bksTGU0Ui3O0xjifaaSqXAjkeflIun2a7a9Zx%2B8ru0wzhXVyA4XbiUVLgSutIFVPAFDpqbZLy9sD8wu3GkwTeczWWG%2FIG8EdCcG64GNckzTHCN6tBxV9DqG7H7%2Bm%2BfN%2B9aVHE25yOy7i6TvRRpm5uNUw0YP%2FjM9P4ipwbW3KCTmQOxkdr4wPZNG%2FP8KLnc0JexpMCPKQ0j%2FtUBiYut63PibAdvcziJmKGV6SnX%2Bd1S9PUQfqDYg4PwKgliljtF%2Fgupzf2Mh6oh37GkDlP06OTPVct7RdoloNzcOdMubLj6WHtHIQ7AflrQVAxAiMpi3Ttj%2B7Tua%2Bue7XanvCTgYaOZa9mhZKUp2o4Xxzbp%2Be0mrSQB7lDSr42tCNDIN5qfbMMv4aa0O5A6OycyrxNaXohNQdlpEsT18oKm7DXBDZSUwoJ%2BwnQY6qQHi%2FmCpyZ3dt4ZmMwfy8eM3XBnV6ouKmK9aRkKoYSAAETfKxuZ1Yh68JprDVNozsADM%2BNlS7jEbndrr0RmmFUSKHsXJ8CiyMWDlm7G1wUBOXcxE65NhLgF3bCS87nQX5IfRipF%2FAUjrEpCr4lCrRb0U%2FN1XMfpzGXRSKb09tsQeUcO35trDZfPHIW70apPjvv%2FNKda1px6LVcrcXMyIZaZPJC2Plm4j3mQP&X-Amz-SignedHeaders=host&X-Amz-Signature=a1d2e528784595db9e52a14b8362a22d274f3b34a197e724f88606dba1adbb4c> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139986