[PATCH] D74133: [compiler-rt] Build with correct ABI (PR38025)

[Petr Hosek via Phabricator via llvm-commits]

phosek accepted this revision.
phosek added a comment.

LGTM as well, sorry about the delay.



================
Comment at: compiler-rt/cmake/Modules/HandleCompilerRT.cmake:4
+# IF not, emit a message and quit.
+macro(check_compiler_rt_library errflag name target library_file)
+  if(errflag OR NOT EXISTS "${library_file}")
----------------
RVP wrote:
> phosek wrote:
> > I'd consider just inlining this, I don't think a bit of code deduplication is worth the extra complexity and I find the current version more difficult to follow.
> I am not sure what is making it difficult to follow. Is it that the macro contains a `return()`? What if we turned the macro into a function `check_if_compiler_rt_library_exists` that sets the `...-NOTFOUND` variable and inlined the `return()` into `find_compiler_rt_library`? That way, we won't need to duplicate all that code.  I have no problem changing it the way you suggest, if you feel strongly that duplicating the code actually makes it easier to understand (but I think it is better not to duplicate).
What I meant is that this function has a single `if`/`else` block where each side just prints a message and sets a variable, it's called only twice and in one of those invocations the error value is always false, so if you inline this function only one side will be duplicated. On the other hand, when reading the code, when you see call to this function you need to switch to read its body and switch back so there's some cognitive overhead. However, this is really minor and just my personal preference, I'm fine if you keep it.


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

https://reviews.llvm.org/D74133