[libc-commits] [PATCH] D159158: [libc] Add a JSON based config option system.

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/CMakeLists.txt:99-100
+#    available and override vars as suitable.
+# All the three steps will not override options already set from the
+# CMake command line.
+set(main_config_file ${LIBC_SOURCE_DIR}/config/config.json)
----------------
Maybe it's clearer with a direct form "CMake command line option takes precedence over 'config.json' options".


================
Comment at: libc/CMakeLists.txt:109
+  if(DEFINED ${opt_name})
+    # The option is alredy defined from the command line so we ignore it here.
+    # We still make note of it so that further config load will also ignore
----------------
typo


================
Comment at: libc/cmake/modules/LibcConfig.cmake:26-28
+  if(NOT EXISTS ${config_file})
+    return()
+  endif()
----------------
Maybe state in the documentation that the function does nothing if the file is missing.


================
Comment at: libc/cmake/modules/LibcConfig.cmake:98-102
+# Loads the config options listed in |config_file|. If any options are not to
+# be overridden, then their names should be passed after |config_file|. For
+# names not present in the not-to-be-overriden list, a cmake var with that
+# name is created if not already present and set to the value listed in the
+# config file.
----------------
This is not super clear, can you rephrase without double negation? Also maybe an exemple would help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159158