[PATCH] D37644: [CMake][runtimes] Use list of lists rather than ":" delimiters

[Chris Bieneman via Phabricator via llvm-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

Sorry for the long delay on this review. It keeps getting lost in my inbox.

I'm totally fine with the patch as-is, but did have some comments below.

I will defer to your judgement on how you think it is best to proceed because you've been doing a phenomenal job with all this.



================
Comment at: runtimes/CMakeLists.txt:349
     foreach(runtime_name ${runtime_names})
-      list(APPEND ${name}_extra_targets
-        "${runtime_name}:${runtime_name}-${name}"
-        "install-${runtime_name}:install-${runtime_name}-${name}")
+      set(${name}_${runtime_name} ${runtime_name} ${runtime_name}-${name})
+      set(${name}_install_${runtime_name} install-${runtime_name} install-${runtime_name}-${name})
----------------
phosek wrote:
> I'm wandering whether we even need list of lists here, we may as well just do `set(${runtime_name}-${name} ${runtime_name})` and in `llvm_ExternalProject_Add` just check if the variable is defined, if so use its value otherwise use it as string. What do you think about that approach?
Your suggestion here sounds like it might be simpler. I'd say go with the simpler approach.


================
Comment at: runtimes/CMakeLists.txt:432
         set(target ${name})
         string(REPLACE ":" ";" target_list ${target})
         list(GET target_list 0 name)
----------------
phosek wrote:
> There's one other use case of ":" which is used in cache files, I'm not sure if we should replace that one as well?
Ideally I think we probably should replace it. In general I don't like treating `:` as special characters.


Repository:
  rL LLVM

https://reviews.llvm.org/D37644