[libc-commits] [PATCH] D159112: [libc] POC for namespace customization

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/CMakeLists.txt:58
+  set(LLVM_LIBC_NAMESPACE "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}")
+endif()
+
----------------
sivachandra wrote:
> Can we replace lines 51 to 58 with:
> 
> ```
> set(LIBC_NAMESPACE
>   "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}"
>   CACHE STRING "The namespace to use to enclose internal implementations. Must start with '__llvm_libc'."
> )
> ```
> 
> The effective changes are:
> 
> 1. Call the CMake variable just `LIBC_NAMESPACE`.
> 2. Since it is a `CACHE` var, it will be set only if it is not being set on the command line.
> 3. We don't need `FORCE` as we do not really want to change the cached value.
I initially did the two step version with `FORCE` to make sure that `CMake` does not retain an old value.

i.e., if we run `cmake -DLIBC_NAMESPACE=__llvm_libc ...` then `cmake ...` I would expect the second run to use the autogenerated namespace value. WDYT?


================
Comment at: libc/cmake/modules/LLVMLibCCompileOptions.cmake:10
+        target_compile_options(${target_name} PRIVATE "/DLIBC_NAMESPACE=${LLVM_LIBC_NAMESPACE}")
+    endif()
+endfunction()
----------------
sivachandra wrote:
> Instead of adding a compile option to every target, can we just do one [[ https://cmake.org/cmake/help/latest/command/add_definitions.html | add_definitions ]] call in `libc/CMakeLists.txt`?
We could. For large projects I tend to prefer being //explicit// rather than //implicit// and //local// rather than //global//.
`LIBC_NAMESPACE` might be a could use of `add_definitions` though.

I think we should still have a file like `LLVMLibCCompileOptions.cmake` for other options like `-fno-expections`, `-ffreestanding` or `-fno-rtti`. We currently have ad-hoc `target_compile_options` all over the place which will fail when compiling with `MSVC` for instance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159112