[libcxx-commits] [PATCH] D108416: [llvm-libgcc] initial commit

Saleem Abdulrasool via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Feb 15 08:10:07 PST 2022


compnerd added inline comments.


================
Comment at: llvm-libgcc/CMakeLists.txt:10
+# Add path for custom modules
+list(INSERT CMAKE_MODULE_PATH 0
+  "${CMAKE_CURRENT_SOURCE_DIR}/cmake"
----------------
Any reason for insert rather that append?


================
Comment at: llvm-libgcc/CMakeLists.txt:36
+set(COMPILER_RT_BUILD_BUILTINS On)
+set(COMPILER_RT_BUILTINS_HIDE_SYMBOLS Off)
+add_subdirectory("../compiler-rt" "compiler-rt")
----------------
I tend to prefer all caps for the `On` and `Off`, but its equivalent really.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:3
+include(GNUInstallDirs)
+string(REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
+
----------------
newline before this line would be nice.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:8
+
+add_custom_command(
+  OUTPUT gcc_s.ver
----------------
I tend to prefer the custom command before the custom target so you can see what the target is generating.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:17
+)
+add_custom_target(gcc_s_version_script DEPENDS gcc_s.ver)
+set_target_properties(
----------------
What differentiates this from the `gcc_s_ver` target? I think that you have two targets for the same output.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:19
+set_target_properties(
+  gcc_s_version_script PROPERTIES
+  OUTPUT_PATH "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver")
----------------
I think that breaking after the `(` is a bit unnecessary:

```
set_target_properties(gcc_s_version_script PROPERTIES
  OUTPUT_PATH "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver")
```

Similar throughout.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:54
+#  COMMAND
+#    ${CMAKE_COMMAND} -E create_symlink builtins "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/libgcc.a")
+
----------------
Dead code?


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:56
+
+set(LLVM_LIBGCC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX} CACHE PATH "")
+install(TARGETS libgcc_s
----------------
Why is this cached?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108416



More information about the libcxx-commits mailing list