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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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()
+
----------------
gchatelet wrote:
> 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?
> I initially did the two step version with `FORCE` to make sure that `CMake` does not retain an old value.

The way I understand it, the `FORCE` on line 52 just clears out the command line arg also?

> 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?

I don't think this is possible in general at all? As in, I am not sure there is a way to overwrite cache variables that way at all other than `FORCE` of course, but `FORCE` erases the command line value the first time as well?

If one is making another CMake run with a different set of command line options, they should cleanup the files from the previous run.  I went looking for an option and found the `--fresh` option but it is available only from 3.24: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-fresh.


================
Comment at: libc/cmake/modules/LLVMLibCCompileOptions.cmake:10
+        target_compile_options(${target_name} PRIVATE "/DLIBC_NAMESPACE=${LLVM_LIBC_NAMESPACE}")
+    endif()
+endfunction()
----------------
gchatelet wrote:
> 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.
> 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.

There cannot be two different namespaces during a single build so I think just adding via `add_definitions` would be sufficient. If we hit problems, we can always make it more complicated at that time.

> 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.

Acknowledged. I think we can do it irrespective of this POC.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159112