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

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.


================
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()
----------------
labath wrote:
> 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.
Sure, fixed the regex.  I think that we need to actually handle this this way unfortunately as otherwise we would not be able to specify the values at the command line to avoid the pkg-config search.


https://reviews.llvm.org/D46726