[PATCH] D57063: [CMake] Unify scripts for generating VCS headers

[Michał Górny via Phabricator via llvm-commits]

mgorny added inline comments.


================
Comment at: clang/lib/Basic/CMakeLists.txt:15
 
+file(WRITE "${version_inc}.undef"
+  "#undef LLVM_REVISION\n"
----------------
Please disregard if you think it's not worth the effort right now but… I'm wondering if you could also move this part of the logic to `GenerateVersionfromVCS`. I mean, the contents resemble those that you are generating inside that script, and you're basically repeating this whole conditional part in both call sites. I suppose you could alos kill the `.undef` file, and just call `GenerateVersionfromVCS` unconditionally, presuming it'll either generate revision data or undefs.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:58
+      if (EXISTS "${git_dir}/svn/refs")
+        execute_process(COMMAND ${GIT_EXECUTABLE} svn info
+          WORKING_DIRECTORY ${path}
----------------
phosek wrote:
> mgorny wrote:
> > phosek wrote:
> > > mgorny wrote:
> > > > Can't you reuse `get_source_info_git_svn` here?
> > > I considered it but that macro does some duplicate work like finding Git and finding the working directory which seems unnecessary.
> > Isn't the result cached? And if it isn't, can you cache it? Avoiding duplicate code makes future maintenance easier, and I think this is exactly the purpose of this diff.
> I'm not sure what you mean by duplicate code? I have removed `get_source_info_git_svn` (the one in the first patch was just a leftover) and instead merged its logic into this macro to avoid duplication. I can split this code into a macro again if you prefer?
Sorry, I haven't looked. It's fine now, yes.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:43
+        if(git_result EQUAL 0)
+          string(REGEX REPLACE "^(.*\n)?Revision: ([^\n]+).*"
+            "\\2" git_svn_rev "${git_output}")
----------------
Hmm, I'm wondering if it's possible to make `subversion_wc_info` to work on top of git-svn.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57063