This is an archive of the discontinued LLVM Phabricator instance.

[lldb/CMake] Rename LLDB_DISABLE_PYTHON to LLDB_ENABLE_PYTHON
ClosedPublic

Authored by JDevlieghere on Dec 13 2019, 10:29 AM.

Details

Summary

This matches the naming scheme used by LLVM and all the other optional dependencies in LLDB.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 13 2019, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 10:29 AM
Herald added subscribers: abidh, mgorny. · View Herald Transcript
mgorny added inline comments.Dec 13 2019, 12:31 PM
lldb/cmake/modules/LLDBConfig.cmake
46

I know that the same thing is done above but… what's the point of setting it to ON again?

JDevlieghere marked an inline comment as done.Dec 13 2019, 12:42 PM
JDevlieghere added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
46

It's just setting the default to the old value. If the value was set, we should honor it, both if it was OFF or ON.

mgorny added inline comments.Dec 13 2019, 12:51 PM
lldb/cmake/modules/LLDBConfig.cmake
46

But it's already set to ON a few lines above.

JDevlieghere marked an inline comment as done.Dec 13 2019, 12:53 PM
JDevlieghere added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
46

Do you mean the DEFINED? That just checks if the variable exists, not its value.

teemperor accepted this revision.Dec 13 2019, 1:03 PM

That seems like a simple mechanical change and the move from DISABLE to ENABLE was IIRC already consensus, so LGTM.

lldb/source/Commands/CommandObjectType.cpp
1173

lol?

This revision is now accepted and ready to land.Dec 13 2019, 1:03 PM
mgorny added inline comments.Dec 13 2019, 1:19 PM
lldb/cmake/modules/LLDBConfig.cmake
46

No, I mean you have:

set(default_enable_python ON)
#...
if (DEFINED LLDB_DISABLE_PYTHON)
  #...
  else()
    set(default_enable_python ON)

This is superfluous since the default is already ON.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
JDevlieghere marked an inline comment as done.Dec 13 2019, 2:07 PM
JDevlieghere added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
46

Got it, thanks. Simplified the code.

lldb/source/Commands/CommandObjectType.cpp
1173

Hahaha

labath added inline comments.Dec 16 2019, 12:31 AM
lldb/cmake/modules/LLDBConfig.cmake
46

Yeah, I added the curses stuff without too much optimization to keep the debian bot working until the master restarts (so the configuration change takes effect). I don't think we have any bots using LLDB_DISABLE_PYTHON, so this probably wasn't even needed, but this all isn't very important -- the code will be gone in a few days anyway...

labath added inline comments.Dec 20 2019, 8:21 AM
lldb/cmake/modules/LLDBConfig.cmake
46

Btw, I've just removed the LLDB_DISABLE_CURSES blurb. If you don't know of any reason to keep LLDB_DISABLE_PYTHON python around (I don't) then I think that can go too...