Page MenuHomePhabricator

[update_cc_test_checks] Don't attach CHECK lines to function declarations
ClosedPublic

Authored by arichardson on Jan 30 2020, 6:44 AM.

Details

Summary

Previously we were adding the CHECK lines to both definitions and
declarations. Update the JSON AST dump parsing code to skip all
FunctionDecls without an "inner" node (i.e. no body).

Diff Detail

Event Timeline

arichardson created this revision.Jan 30 2020, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2020, 6:44 AM

Remove debug output

Unit tests: fail. 62344 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay added inline comments.Jan 30 2020, 9:50 AM
llvm/test/tools/UpdateTestChecks/update_cc_test_checks/def-and-decl.test
4

-f is not needed.

5

diff -u -> cmp

llvm/utils/update_cc_test_checks.py
80

Does 'inner' in node work?

arichardson marked 4 inline comments as done.Jan 31 2020, 2:53 AM
arichardson added inline comments.
llvm/test/tools/UpdateTestChecks/update_cc_test_checks/def-and-decl.test
5

I prefer diff -u so that you get useful output on failure. It's using the lit-internal diff so it's portable.

llvm/utils/update_cc_test_checks.py
80

Yes, changed it. Thanks.

arichardson marked an inline comment as done.

Address feedback

Unit tests: pass. 62362 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

LGTM. I did something similar in my local copy that I was going to upstream but I added a switch for the user to choose where to place checks (declaration, definition or both). I only added the switch because I wasn't sure whether the changed behavior was intended. I'm fine just placing checks on definitions as I don't have a need to put them on declarations.

greened accepted this revision.Feb 3 2020, 6:13 PM
This revision is now accepted and ready to land.Feb 3 2020, 6:13 PM
MaskRay accepted this revision.Feb 3 2020, 6:24 PM
This revision was automatically updated to reflect the committed changes.