jdenny added a comment.
In D104714#2834696 <https://reviews.llvm.org/D104714#2834696>, @arichardson
wrote:
> Thanks for working on this!
Thanks for the review.
> We have some tests downstream that check globals and currently have to use
> `// UTC_ARGS: --disable` to manually retain them.
Does that mean you manually maintain FileCheck directives there?
> The other update script tests compare to an expected output file instead of
> using Filecheck directives. For larger tests the separate files are a lot
> more readable, whereas it doesn't really matter for this test.
I believe the only reason I used FileCheck directives in this case is because I
didn't want to hard-code expected metadata and attributes, so I wanted regexes.
However, I don't have a lot of experience writing checks for such constructs,
so maybe hard-coding those is not brittle at all. If so, I'd be happy to
switch to expected output files, which I agree are more readable.
================
Comment at: clang/test/utils/update_cc_test_checks/check-globals.test:34
+RUN: %lit %t
+# Lit was successful. Sanity-check the results.
+RUN: %lit %t 2>&1 | FileCheck -check-prefix=LIT-RUN %s
----------------
arichardson wrote:
> Running lit to verify that the output is valid could be something we might
> want to do for the other update script tests. But I guess using the scripts
> to generate tests that are checked it is usually sufficient.
I don't know a general rule for when it's worthwhile and when it's not.
In this patch, I felt it was worthwhile because it was easy to think the
directives looked fine without realizing declarations were in the wrong order
(that happened to me at one point during development). I'm not sure someone
modifying update_cc_test_checks.py and running this test suite is necessarily
going to know which tests in other test suites should be re-generated using
update_cc_test_checks.py and re-run to be sure all is well.
On the other hand, I think I could be convinced it's not worthwhile in my later
patches in this series.
================
Comment at: llvm/utils/update_cc_test_checks.py:357
continue # Don't append the existing CHECK lines
+ if line.strip() == '//' + common.SEPARATOR:
+ continue
----------------
arichardson wrote:
> I feel like this line could do with a comment since it's not immediately
> obvious why it's needed as part of this commit.
Sure, I'll work on that.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104714/new/
https://reviews.llvm.org/D104714
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
