[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

Tue Jun 22 16:01:45 PDT 2021

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