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

[Greg Bedwell via Phabricator via llvm-commits]

gbedwell added inline comments.


================
Comment at: utils/update_mca_test_checks.py:445
+  # The file should not end with two newlines. It creates unnecessary churn.
+  while len(output_lines) > 0 and output_lines[-1] == '':
+    output_lines.pop()
----------------
lebedev.ri wrote:
> gbedwell wrote:
> > can be simplified to 
> > ```while output_lines and output_lines[-1] == '':```
> > I think.
> 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.
I don't feel strongly about it.


================
Comment at: utils/update_mca_test_checks.py:458
   with open(test_path, 'wb') as f:
-    for line in output_lines:
-      f.write('{}\n'.format(line.rstrip()).encode())
-
+    f.writelines([l + '\n' for l in output_lines])
 
----------------
lebedev.ri wrote:
> gbedwell wrote:
> > 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 update_mca_test_checks.py ..\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 "update_mca_test_checks.py", line 499, in <module>
> >     sys.exit(main())
> >   File "update_mca_test_checks.py", line 491, in main
> >     prefix_pad)
> >   File "update_mca_test_checks.py", line 458, in _write_output
> >     f.writelines([l + '\n' for l in output_lines])
> > TypeError: a bytes-like object is required, not 'str'
> > ```
> 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?
> 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])
```




Repository:
  rL LLVM

https://reviews.llvm.org/D47697