[PATCH] D139006: [UpdateTestChecks] Match define for labels

arichardson added a comment. I like the idea of defaulting to --function-signature for new tests a lot, so I've implemented that in D140212 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139006/new/ https://review

[PATCH] D139006: [UpdateTestChecks] Match define for labels

spatel added a comment. In D139006#4000992 , @sebastian-ne wrote: > Adding `--function-signature` by default sounds like a good idea to me. > Is there any reason why we wouldn’t want to enable this by default (for new > tests)? No objection from me for

[PATCH] D139006: [UpdateTestChecks] Match define for labels

sebastian-ne added a comment. > I believe the motivation here is the default behavior Correct, update_test_checks produces a broken test for some input and I think this is a bug that we should fix. Sorry for the test churn and thanks for the revert (I only got to work a few hours after nikic a

[PATCH] D139006: [UpdateTestChecks] Match define for labels

nikic added a comment. In D139006#384 , @jdoerfert wrote: > Why not just `--function-define` as a way to enable the `define` but not the > `captures`, if that is really something necessary. > It is unclear to me why `function-signature` is not suffic

[PATCH] D139006: [UpdateTestChecks] Match define for labels

jdoerfert added a comment. Why not just `--function-define` as a way to enable the `define` but not the `captures`, if that is really something necessary. It is unclear to me why `function-signature` is not sufficient here and what the big problem with using it is. Repository: rG LLVM Github

[PATCH] D139006: [UpdateTestChecks] Match define for labels

spatel added subscribers: arsenm, spatel. spatel added a comment. I agree with the revert - we'd need lots of commits like this: b7232dafe69eb04c14217 (cc @arsenm) Another possibility to fix this with less churn: Add a warnin

[PATCH] D139006: [UpdateTestChecks] Match define for labels

nikic added a comment. In D139006#3988227 , @sebastian-ne wrote: > In D139006#3988215 , @mkazantsev > wrote: > >> So now every single test needs to be regenerated? It'll create straw diff >> from nowhere... > >

[PATCH] D139006: [UpdateTestChecks] Match define for labels

sebastian-ne added a comment. In D139006#3988215 , @mkazantsev wrote: > So now every single test needs to be regenerated? It'll create straw diff > from nowhere... We don’t need to regenerate every test. Similar to how we don’t reformat all of LLVM aft

[PATCH] D139006: [UpdateTestChecks] Match define for labels

mkazantsev added a comment. So now every single test needs to be regenerated? It'll create straw diff from nowhere... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139006/new/ https://reviews.llvm.org/D139006 _

[PATCH] D139006: [UpdateTestChecks] Match define for labels

This revision was automatically updated to reflect the committed changes. Closed by commit rGa25aeef8: [UpdateTestChecks] Match define for labels (authored by sebastian-ne). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139006/new/ https://revie

[PATCH] D139006: [UpdateTestChecks] Match define for labels

lebedev.ri added a subscriber: nikic. lebedev.ri added a comment. In D139006#3981326 , @sebastian-ne wrote: > I guess this is fine to merge. I’ll leave it for a day in case someone has > more comments. I guess, we could be pedantic and post an RFC to d

[PATCH] D139006: [UpdateTestChecks] Match define for labels

sebastian-ne added a comment. I guess this is fine to merge. I’ll leave it for a day in case someone has more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139006/new/ https://reviews.llvm.org/D139006 ___

[PATCH] D139006: [UpdateTestChecks] Match define for labels

sebastian-ne added inline comments. Comment at: llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/define_after_call.ll.expected:14 +define i32 @b(i32 %arg) { +; CHECK-LABEL: define {{[^@]+}}@b( +; CHECK-NEXT:ret i32 [[ARG:%.*]] arichardson wrote: >

[PATCH] D139006: [UpdateTestChecks] Match define for labels

arichardson added a comment. Agreed that the churn is annoying, but at least unlike the function-signature flag (which I'd quite like to have on by default tbh) it only affects a single line rather than also including variable captures. Comment at: llvm/test/tools/UpdateTest

[PATCH] D139006: [UpdateTestChecks] Match define for labels

lebedev.ri added a comment. In D139006#3960466 , @mtrofin wrote: > LGTM, but I'd wait for Johannes to review it, too (because of > e67f6477fd1ed29acbeddf8482c25d8db826912f > . I for >

[PATCH] D139006: [UpdateTestChecks] Match define for labels

mtrofin accepted this revision. mtrofin added a comment. LGTM, but I'd wait for Johannes to review it, too (because of e67f6477fd1ed29acbeddf8482c25d8db826912f . I for one don't quite follow the reasoning there wrt adding the

[PATCH] D139006: [UpdateTestChecks] Match define for labels

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. LGTM. I do think this is an important fix, not the least because i've also just hit that, but i do want to explicitly call out that this will cause a massive test case churn, so i do w

[PATCH] D139006: [UpdateTestChecks] Match define for labels

sebastian-ne updated this revision to Diff 478933. sebastian-ne added a comment. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, jdoerfert, sstefan1. Herald added a project: clang. Thanks for the review! I updated the update_cc_tests tests and added a test where the File