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