labath accepted this revision. labath added a comment. This revision is now accepted and ready to land.
Looks fine to me. Just remove the redundant panel searching code, and please make sure this works correctly from a clean build, per the inline comment. ================ Comment at: lldb/cmake/modules/FindCursesAndPanel.cmake:8 +if(CURSES_INCLUDE_DIRS AND CURSES_LIBRARIES AND PANEL_LIBRARIES) + set(CURSES_PANEL_FOUND TRUE) +else() ---------------- It's not fully clear to me what will happen when this code is run for the first time (when `CURSES_INCLUDE_DIRS`, etc. is not defined yet). Who will set `CURSES_PANEL_FOUND` in that case? Could you make sure this works correctly when run for the first time on a fully clean build? I don't know whether this is the standard way of writing find_package files, but I'd consider just removing the caching and letting `find_package(Curses)` and `find_library(panel)` just run every time -- they already contain some internal caching so we're not saving much here anyway... ================ Comment at: lldb/cmake/modules/LLDBConfig.cmake:497 if (LLDB_ENABLE_CURSES) - find_package(Curses REQUIRED) find_library(CURSES_PANEL_LIBRARY NAMES panel DOC "The curses panel library") if (NOT CURSES_PANEL_LIBRARY) ---------------- mgorny wrote: > Isn't this redundant now? yep. it sure is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71306/new/ https://reviews.llvm.org/D71306 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits