[PATCH] D49268: [clang-doc] Create a script to generate tests

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Nice!
Some comments.
Sorry about lack of the review, this kinda fell off my radar.

Did you meant to commit `clang-tools-extra/clang-doc/test_cases/` ?
I can't tell whether it is a temporary directory or not.



================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:1
+#!/usr/bin/env python
+#
----------------
Does it work with python3?
If yes, i wonder if it would be a good idea to hardcode python3 requirement from the start?


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:62
+  test_file = os.path.join(test_cases_path, 'test.cpp')
+  shutil.copyfile(test_case_path, test_file)
+  return test_file
----------------
Ah, i see, so this happens sequentially, using the same source location for each test.


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:72-75
+  return_code = subprocess.call(current_cmd)
+  if return_code:
+    print('Error running clang-doc on ' + os.path.basename(test_case_path))
+    return 1
----------------
The error msg will already be dumped to screen?


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:116-127
+  parser = argparse.ArgumentParser(description='Generate clang-doc tests.')
+  parser.add_argument('-flag', action='append', default=[], dest='flags',
+                      help='Flags to pass to clang-doc.')
+  parser.add_argument('-prefix', type=str, default='', dest='prefix',
+                      help='Prefix for this test group.')
+  parser.add_argument('-clangdoc', dest='clangdoc',
+                      default='clang-doc',
----------------
The `llvm/utils/update_*_test_checks.py` seem to prefer `--*-binary` syntax.
I wonder whether consistency is good.


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:131-133
+  clang_doc_path = os.path.dirname(__file__)
+  test_cases_path = os.path.join(clang_doc_path, 'test_cases')
+  tests_path = os.path.join(clang_doc_path, '..', 'test', 'clang-doc')
----------------
Will the script work regardless of the `pwd` it is called from?


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:138
+  for test_case_path in glob.glob(os.path.join(test_cases_path, '*')):
+    if test_case_path.endswith('compile_flags.txt'):
+      continue
----------------
Probably also `compile_commands.json`


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:142
+    # Name of this test case
+    case_name = os.path.basename(test_case_path).replace('.cpp', '')
+
----------------
Any plans supporting docs for C code?
Maybe you want to just drop the last filename suffix.


================
Comment at: clang-tools-extra/clang-doc/gen_tests.py:156
+        # Make the file check the first 3 letters (there's a very small chance
+        # that this will collide, but the fix is to simply chance the decl name)
+        usr = os.path.basename(out_file).split('.')
----------------
s/chance/change/


https://reviews.llvm.org/D49268