[PATCH] D66526: WIP: [utils] Add the llvm-locstats tool

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

@vsk

> I really like this approach. Thanks for doing this!

Thanks!



================
Comment at: llvm/utils/llvm-locstats/llvm-locstats.py:107
+  # Get the llvm-dwarfdump --statistics and write it into the temproary file.
+    tmp_stats_file = 'stats.json'
+    run_command = \
----------------
Orlando wrote:
> vsk wrote:
> > Please use subprocess.call or Popen along with PIPE to avoid round-tripping to the filesystem, leaving stale files around by accident, etc.
> Would it be worth using `tempfile.NamedTemporaryFile`  instead of hard coding the temporary file name here?
>Please use subprocess.call or Popen along with PIPE to avoid round-tripping to the filesystem, leaving stale files around by accident, etc.
Sure.

>Would it be worth using tempfile.NamedTemporaryFile instead of hard coding the temporary file name here?
I think so, thanks!


================
Comment at: llvm/utils/llvm-locstats/llvm-locstats.py:109
+    run_command = \
+        wd + '/llvm-dwarfdump --statistics ' \
+        + binary + ' > ' + tmp_stats_file
----------------
vsk wrote:
> Please use os.path.join to support Windows.
Sure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66526/new/

https://reviews.llvm.org/D66526