labath added a comment.

The patch looks quite nice, though I can't say I know much about how 
find_package modules are supposed to be written. I won't click accept yet to 
see if anyone with more cmake knowledge steps in to review. In the mean time, I 
have two nitty 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
----------------
Should we make the capitalization of these match the file name (LibEdit_FOUND) ?


================
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()
----------------
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..


https://reviews.llvm.org/D46726



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to