[PATCH] D54678: [gn build] Create abi-breaking.h, config.h, llvm-config.h, and add a build file for llvm/lib/Support.

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/utils/gn/build/libs/zlib/enable.gni:1
+llvm_enable_zlib = host_os != "win"
----------------
Shouldn't this be an arg as well?  It's an option today and various builds disable it.


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:48
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--values', nargs='*')
----------------
Can you pass the documentation above as `description` and `epilog` to the ArgumentParser so it gets printed out with `--help`?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:49
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--values', nargs='*')
+    parser.add_argument('input')
----------------
Shouldn't this be just a positional argument?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:50
+    parser.add_argument('--values', nargs='*')
+    parser.add_argument('input')
+    parser.add_argument('-o', '--output')
----------------
Would it be possible to add description for all arguments?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:51
+    parser.add_argument('input')
+    parser.add_argument('-o', '--output')
+    args = parser.parse_args()
----------------
What if this argument is unset?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:62
+    # Matches e.g. '${CLANG_FOO}' and captures CLANG_FOO in group 1.
+    var_re = re.compile(r'\$\{([^}]*)\}')
+
----------------
Make it a global constant?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:64
+
+    in_lines = open(args.input).readlines()
+    out_lines = []
----------------
Wrap in `with` statement so it gets closed automatically.


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:102
+    if not os.path.exists(args.output) or open(args.output).read() != output:
+        open(args.output, 'w').write(output)
+
----------------
Ditto


================
Comment at: llvm/utils/gn/secondary/llvm/lib/Target/targets.gni:43
+  }
+}
+
----------------
This whole block seems unused.


================
Comment at: llvm/utils/gn/secondary/llvm/triples.gni:10
+# FIXME: This should probably become a declare_args eventually.
+llvm_target_triple = llvm_host_triple
----------------
Definitely, we always set host triple to `x86_64-linux-gnu` in our build (because nobody has time to type `unknown`)


https://reviews.llvm.org/D54678