drodriguez requested changes to this revision.
drodriguez added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/lib/Basic/CMakeLists.txt:16
set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
endif()
if(clang_vc AND LLVM_APPEND_VC_REV)
----------------
I imagine you do not need it, but maybe LLVM should have the same changes?
There's a `for` loop in `GenerateVersionFromVCS.cmake` that is already going to
try to look into `LLVM_VC_REPOSITORY/REVISION`.
================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:21
-function(append_info name path)
- if(path)
- get_source_info("${path}" revision repository)
+function(append_info name path repository revision)
+ if (NOT repository AND NOT revision)
----------------
While I agree that `repository` and then `revision` makes more sense, the
existing `get_source_info` receives them in the opposite order.
================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:23-25
+ if(path)
+ get_source_info("${path}" revision repository)
+ endif()
----------------
Nit: Looks like tabs were added?
================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:47-49
+ if(NOT DEFINED ${name}_SOURCE_DIR)
+ message(FATAL_ERROR "${name}_SOURCE_DIR is not defined")
+ endif()
----------------
Tabs?
================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:50
+ endif()
+ append_info(${name} "${${name}_SOURCE_DIR}" "" "")
endif()
----------------
I would recommend changing the flow a little bit:
- Remove `path` argument from `append_info`. Leave `name`, `revision`, and
`repository`.
- Remove the `if(path) get_source_info() endif()` part.
- Change this `foreach` to the following:
```
foreach(name IN LISTS NAMES)
if(${name}_VC_REPOSITORY AND ${name}_VC_REVISION)
set(revision ${${name}_VC_REVISION})
set(repository ${${name}_VC_REPOSITORY})
elseif(DEFINED ${name}_SOURCE_DIR)
get_source_info(${${name}_SOURCE_DIR} revision repository)
else()
message(FATAL_ERROR "${name}_SOURCE_DIR is not defined")
endif()
append_info(${name} ${revision} ${repository})
endforeach()
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148262/new/
https://reviews.llvm.org/D148262
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
