[Lldb-commits] [PATCH] D46726: build: use cmake to find the libedit content

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: cmake/modules/FindLibEdit.cmake:11-14
+#   libedit_FOUND         - true if libedit was found
+#   libedit_INCLUDE_DIRS  - include search path
+#   libedit_LIBRARIES     - libraries to link
+#   libedit_VERSION       - version number
----------------
compnerd wrote:
> labath wrote:
> > Should we make the capitalization of these match the file name (LibEdit_FOUND) ?
> *shrug* I'm not tied to that.  I found `LibEdit_FOUND` to be a bit jarring, which is why I went with `libedit_FOUND` which also mirror's the project's actual spelling (`libedit`).
Ok, whatever you prefer.


================
Comment at: cmake/modules/FindLibEdit.cmake:53-54
+    set(libedit_VERSION_STRING "${libedit_major_version}.${libedit_minor_version}")
+    unset(libedit_major_version)
+    unset(libedit_minor_version)
+  endif()
----------------
compnerd wrote:
> labath wrote:
> > Do you really need to unset these? I would hope that this file is evaluated in a fresh context, and it won't pollute the callers namespace or anything..
> I'm pretty sure that they get evaluated in the global context :-(.
I just tried removing these and putting a `message` command after the `find_package` call. The variables did not get exported.

However, I think we have a bigger issue here. If I apply your patch without any modifications, I get an error first time I run it because of unrecognised sequences in the regular expressions (`\s`, `\d`). I guess that's why you've changed these to `[ \t]` and `[0-9]` in the first regex.

What particularly worries me is that the second time I ran cmake, without any modifications, it succeeded, presumably because we took the true branch in the `if` statement at the top. This looks wrong.

I think we should just remove the if check -- `find_path` and `find_library` should already have caching internally, and if you want to cache the version computation, then that should be guarded by a separate variable.


https://reviews.llvm.org/D46726