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

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/utils/gn/build/libs/zlib/enable.gni:1
+llvm_enable_zlib = host_os != "win"
----------------
phosek wrote:
> Shouldn't this be an arg as well?  It's an option today and various builds disable it.
Done. I agree it should match llvm_enable_libxml2 which I did make a declare_arg. I actually think it'd make more sense to say "llvm requires libxml2 and zlib on non-Win" instead of dynamically disabling features, but probably makes sense to match cmake.

Out of interest, do builds you care about disable this?


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:49
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--values', nargs='*')
+    parser.add_argument('input')
----------------
phosek wrote:
> Shouldn't this be just a positional argument?
Is there some guideline what to use for what? Changed; I had it the current way because I found the command easier to read when looking at it with `ninja -v`.


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


================
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'\$\{([^}]*)\}')
+
----------------
phosek wrote:
> Make it a global constant?
Why? It's only used here.


================
Comment at: llvm/utils/gn/build/write_cmake_config.py:64
+
+    in_lines = open(args.input).readlines()
+    out_lines = []
----------------
phosek wrote:
> Wrap in `with` statement so it gets closed automatically.
It's a short and short-running script, the string will be garbage-collected, and I don't need to indent everything 4 more (or introduce a function that's called from just one place.


================
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)
+
----------------
phosek wrote:
> Ditto
This is for consistency with the previous place :-)


================
Comment at: llvm/utils/gn/secondary/llvm/lib/Target/targets.gni:43
+  }
+}
+
----------------
phosek wrote:
> This whole block seems unused.
>From the phab description: "I'm also adding llvm/utils/gn/secondary/llvm/lib/Target/targets.gni in this patch because $native_arch is needed for writing llvm-config.h -- the rest of it will be used later, when the build files for llvm/lib/Target get added. That file describes how to select which archs to build."

Want me to defer adding the bits I need later until I need them?


https://reviews.llvm.org/D54678