This is an archive of the discontinued LLVM Phabricator instance.

[UpdateTestChecks] Emit warning when invalid test paths
ClosedPublic

Authored by xbolva00 on Jul 11 2019, 9:26 AM.

Details

Summary

Recently I ran into the following issue:

./update_test_checks.py /path/not-existing-file.ll

The script was silent and I was suprised why the real test file hadn't been updated.

Solution:
Emit warning if we detect this problem.

Diff Detail

Event Timeline

xbolva00 created this revision.Jul 11 2019, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 9:26 AM
jdoerfert accepted this revision.Jul 11 2019, 9:35 AM
jdoerfert added a subscriber: jdoerfert.

LGTM.

This revision is now accepted and ready to land.Jul 11 2019, 9:35 AM
xbolva00 updated this revision to Diff 209250.Jul 11 2019, 9:41 AM

Improve code, emit warning instead of error.

LGTM.

Do you think hard error is better? I switched to warning since if somebody use

./update_test_checks.py fileA fileB fileC

and fileC does not exists, the script will exit.

New behaviour, we process fileA and fileB and we emit warnign for fileC.

xbolva00 retitled this revision from [UpdateTestChecks] Emit error when invalid test paths to [UpdateTestChecks] Emit warning when invalid test paths.Jul 11 2019, 9:43 AM
xbolva00 edited the summary of this revision. (Show Details)

I personally think hard error may be worse.

Maybe @spatel would like to leave a comment too?

nikic accepted this revision.Jul 11 2019, 12:29 PM

Your current variant with the warning looks good to me.

lebedev.ri accepted this revision.Jul 11 2019, 12:34 PM

(didn't see i'm on the review list here)

spatel accepted this revision.Jul 11 2019, 1:05 PM

LGTM

Thank you all!

This revision was automatically updated to reflect the committed changes.