labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
I like where this is going, but I think this still needs some work wrt. the
panel library (and being able to customize the dependency search more).
================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:28
+function(add_optional_dependency variable description package found)
+ set(${variable} "AUTO" CACHE STRING "${description} On, Off or Auto
(default)")
+ string(TOUPPER "${${variable}}" ${variable})
----------------
Use the same capitalization of "auto" as the description
================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:41-47
+ # We could set ${variable} directory to ${${found}} but then the value is
+ # TRUE/FALSE rather than ON/OFF.
+ if (${${found}})
+ set(${variable} ON PARENT_SCOPE)
+ else()
+ set(${variable} OFF PARENT_SCOPE)
+ endif()
----------------
I don't care that much about that, but I don't see the advantage in
canonicalizing to ON/OFF (the other variables don't go through the
canonicalization process, so only their default values will be ON/OFF -- the
actual values will be whatever flavour of "true" the user chooses to pass to
cmake)
================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:507
if (NOT CURSES_PANEL_LIBRARY)
- message(FATAL_ERROR "A required curses' panel library not found.")
+ set(LLDB_ENABLE_CURSES OFF)
+ message(WARNING "Curses panel library not found.")
----------------
I was wondering above if the name we pass to the `find_package` provides
sufficient customization, and I think this shows it doesn't. Ideally, the panel
(sub)library would behave the same way as the curses library and respect all
three values of the cache variable. This one does not support the "force on"
mode.
Having the cache variable handling code centralized in a single function is a
worthy goal, but I am not sure that this is really possible, given the current
cmake abilities. e.g. we'd need some sort of a indirect call so we can
implement custom searching logic, but there aren't any particularly nice ways
of achieving that. The only way I know of achieving indirection is to put the
logic in a separate file, and then use either an `include`, `find_package` or
something else to load it.
If you want to go the file loading approach, or "outline" than function (at
least some parts of it), then I think both are fine, but I don't think leaving
this code in here is good, since the whole point of this exercise is to
standardize things.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71306/new/
https://reviews.llvm.org/D71306
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
