Page MenuHomePhabricator

[CMake][runtimes] Use variables rather than ":" delimiters
ClosedPublic

Authored by phosek on Sep 8 2017, 3:37 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Sep 8 2017, 3:37 PM
phosek added inline comments.Sep 8 2017, 3:40 PM
runtimes/CMakeLists.txt
432 ↗(On Diff #114443)

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?

phosek added inline comments.Sep 8 2017, 3:49 PM
runtimes/CMakeLists.txt
349 ↗(On Diff #114443)

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?

ping again?

beanz accepted this revision.Nov 13 2017, 3:56 PM

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.

runtimes/CMakeLists.txt
349 ↗(On Diff #114443)

Your suggestion here sounds like it might be simpler. I'd say go with the simpler approach.

432 ↗(On Diff #114443)

Ideally I think we probably should replace it. In general I don't like treating : as special characters.

This revision is now accepted and ready to land.Nov 13 2017, 3:56 PM
phosek updated this revision to Diff 127749.Dec 20 2017, 9:40 AM
phosek marked 2 inline comments as done.
phosek retitled this revision from [CMake][runtimes] Use list of lists rather than ":" delimiters to [CMake][runtimes] Use variables rather than ":" delimiters.
phosek marked 2 inline comments as done.Dec 21 2017, 12:00 PM
phosek added inline comments.
runtimes/CMakeLists.txt
432 ↗(On Diff #114443)

I'll do this in a separate patch since it'll require more changes.

phosek marked 2 inline comments as done.Jun 25 2018, 12:55 PM
phosek added inline comments.
runtimes/CMakeLists.txt
432 ↗(On Diff #114443)

Done in D48061.

This revision was automatically updated to reflect the committed changes.
phosek marked an inline comment as done.