Page MenuHomePhabricator

[llvm-mca] Make sure not to end the test files with an empty line.

Authored by lebedev.ri on Jun 4 2018, 12:05 AM.



It's super irritating.

[properly configured] git client then complains about that double-newline,
and you have to use --force to ignore the warning, since even if you
fix it manually, it will be reintroduced the very next runtime :/

Diff Detail


Event Timeline

lebedev.ri created this revision.Jun 4 2018, 12:05 AM

Thanks for the patch! Annoying that my git client never complained about this one to me.

445 ↗(On Diff #149677)

can be simplified to

while output_lines and output_lines[-1] == '':

I think.

458 ↗(On Diff #149677)

I've been trying to keep this file compatible with Python 2 and 3, ready for inevitably switching the default at some point hence jumping through a few weird hoops with this sort of stuff. Python 3 now produces:

e:\work\upstream-llvm\llvm\utils>py -3 ..\test\tools\llvm-mca\*\*.s
Test: ..\test\tools\llvm-mca\ARM\simple-test-cortex-a9.s
      [38 lines total]
Traceback (most recent call last):
  File "", line 499, in <module>
  File "", line 491, in main
  File "", line 458, in _write_output
    f.writelines([l + '\n' for l in output_lines])
TypeError: a bytes-like object is required, not 'str'

@gbedwell thank you for taking a look.
I forgot to check git shortlog of the script itself, thus did not add you as a reviewer :)

445 ↗(On Diff #149677)

Elsewhere in this script, len(list) > 0 is used.
I can change it if you insist, but i think this is inconsistent, and less readable.

458 ↗(On Diff #149677)

Works fine here in linux with

$ python3 --version
Python 3.6.5

This change shouldn't be needed, but that is what every single other update script does already.
So are they all broken? Or maybe something on your side?

gbedwell added inline comments.Jun 4 2018, 4:14 AM
445 ↗(On Diff #149677)

I don't feel strongly about it.

458 ↗(On Diff #149677)

So are they all broken?

Yes. I think so :)

My guess is that the difference we're seeing is to do with differing system line endings.

Looking at this a bit more, I think the issue is that the file is opened in binary mode for the call the writelines. If we change the mode on the open to 'w' then it works fine for me on Windows with Python 2 and 3, but this still isn't the right solution as it now starts producing windows line endings on Windows systems.

Given that, this seems to do the trick on Windows, although I've not yet had a chance to verify it on Linux:

with open(test_path, 'wb') as f:
  f.writelines(['{}\n'.format(l).encode() for l in output_lines])
lebedev.ri updated this revision to Diff 149717.Jun 4 2018, 4:23 AM
lebedev.ri marked 3 inline comments as done.

Use new, third variant of writing lines.
FIXME: either there is some other problem elsewhere, or other scrips need to be updated, too.

458 ↗(On Diff #149677)

Ok, that seems to work here.

gbedwell accepted this revision.Jun 4 2018, 4:39 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jun 4 2018, 4:39 AM

LGTM. Thanks!

Thank you for the review, and the script!

This revision was automatically updated to reflect the committed changes.