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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
