[PATCH] D107041: [Flang] Ported test_symbols to Python

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: flang/test/Semantics/OpenACC/acc-symbols01.f90:1
-! RUN: %S/../test_symbols.sh %s %t %flang_fc1 -fopenacc
-! REQUIRES: shell
+! RUN: python %S/../test_symbols.py %s %t %flang_fc1 -fopenacc
 
----------------
The python used here should be the `PYTHON_EXECUTABLE` discovered by cmake, not the one in the default path. Typically, the lit.cfg.py determines what the correct python executable. You could define a placeholder `%test_symbols` in lit.cfg.py that expands to the correct python executable and `test_symbols.py` program (similar to ` %flang_fc1` or `%clang_cc1` by clang). Otherwise, I think llvm-lit even expands some standard tools itself, I know it does in LLVM for `opt`. Could you confirm this is done for `python`?


================
Comment at: flang/test/Semantics/common.py:9-13
+class Common:
+    """Provides common functionality to the test scripts
+    Expects source files passed as first argument;
+    A temporary directory as second argument;
+    And the Flang frontend driver with options as third argument"""
----------------
[not a change request] It does not seem necessary to make this a class. I would understand if `test_symbols.py` would derive from it to e.g. customize `main`, but now its a mixed of object-oriented and script style.


================
Comment at: flang/test/Semantics/common.py:66-68
+if __name__ == '__main__':
+    program = Common(sys.argv)
+    program.main()
----------------
AFAIU, this file is never executed directly. Could you remove this?


================
Comment at: flang/test/Semantics/test_symbols.py:21-23
+src1 = PurePath(tmp).joinpath("1.f90")
+src2 = PurePath(tmp).joinpath("2.f90")
+src3 = PurePath(tmp).joinpath("3.f90")
----------------
This script write intermediate strings into temporary files just to read them back in again. I don't see reason to do that.


================
Comment at: flang/test/Semantics/test_symbols.py:24
+src3 = PurePath(tmp).joinpath("3.f90")
+diffs = PurePath(tmp).joinpath("diffs")
+
----------------
`diffs` is written to, but never used.


================
Comment at: flang/test/Semantics/test_symbols.py:32
+
+# Strips out blak lines and all comments except for "!DEF:", "!REF:", and "!$omp"
+with open(src, 'r') as text_in, \
----------------
[typo] bla**n**k


================
Comment at: flang/test/Semantics/test_symbols.py:52
+with open(src3, 'w') as text_out:
+    proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
+    text = proc.communicate()[0]
----------------
Prefer `subprocess.run` or `subprocess.check_output`


================
Comment at: flang/test/Semantics/test_symbols.py:72
+    print(diff_check)
+    print("")
+    print("FAIL")
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107041