[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 accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

Few more nits, but otherwise LGTM.



================
Comment at: llvm/utils/gn/secondary/llvm/lib/Target/targets.gni:43
+  }
+}
+
----------------
thakis wrote:
> 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?
I missed that part, I guess this is fine then.


================
Comment at: llvm/utils/gn/secondary/llvm/lib/Target/targets.gni:34
+  # FIXME: Port the remaining targets.
+  assert(target == "AArch64" || target == "ARM" || target == "X86",
+         "Unknown target '$target'.")
----------------
Move this assert into an `else` branch below? That way you don't have repeat all targets here and in the `if` conditions below.


================
Comment at: llvm/utils/gn/secondary/llvm/lib/Target/targets.gni:53
+} else {
+  assert(false)
+}
----------------
Could you also add an error message to make it more clear what's wrong?


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

https://reviews.llvm.org/D54678