Page MenuHomePhabricator

Remove MIUtilParse (no longer used)
ClosedPublic

Authored by mgorny on Aug 25 2016, 10:33 AM.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 69268.Aug 25 2016, 10:33 AM
mgorny retitled this revision from to Remove MIUtilParse (no longer used).
mgorny updated this object.
mgorny added a reviewer: dawn.
mgorny added a subscriber: lldb-commits.
mgorny added a subscriber: labath.

@labath, since you committed D23882, could you, please, take a look at this one as well? It's immediately following that, and should fix the build issue resulting from use of private headers.

krytarowski accepted this revision.Sep 10 2016, 2:38 PM
krytarowski added a reviewer: krytarowski.
This revision is now accepted and ready to land.Sep 10 2016, 2:38 PM

It looks good. Remaining TODO for standalone builds:

  • sanitize six.py usage (it's installed into system-wide directory, where standard py-six lands)
  • bump cmake_minimum_required(VERSION 2.8.12.2) to 3.4.3 in cmake/modules/LLDBStandalone.cmake

Final bits for Windows:

  • eliminate the usage of ../lib/Support/regex_impl.h in RegularExpression.h

It looks good. Remaining TODO for standalone builds:

  • sanitize six.py usage (it's installed into system-wide directory, where standard py-six lands)

How would you prefer handling it? Can we just kill it and rely on user installing it system-wide or via pip?

  • bump cmake_minimum_required(VERSION 2.8.12.2) to 3.4.3 in cmake/modules/LLDBStandalone.cmake Final bits for Windows:
  • eliminate the usage of ../lib/Support/regex_impl.h in RegularExpression.h

Wouldn't it be actually better to kill that regex implementation as well, and use the class provided by LLVMSupport everywhere? I started with the other since it caused direct build issues on Linux but I think I could try to kill this one as well.

It looks good. Remaining TODO for standalone builds:

  • sanitize six.py usage (it's installed into system-wide directory, where standard py-six lands)

How would you prefer handling it? Can we just kill it and rely on user installing it system-wide or via pip?

  1. Patch scripts/Python/finishSwigPythonLLDB.py and install six into lldb subdir with a custom name (lldb_six.py).
  2. Patch the Python sources to make use of lldb_six.

Unfortunately there is resistance to grab system-wide (provided by a user) six module.

  • bump cmake_minimum_required(VERSION 2.8.12.2) to 3.4.3 in cmake/modules/LLDBStandalone.cmake Final bits for Windows:
  • eliminate the usage of ../lib/Support/regex_impl.h in RegularExpression.h

Wouldn't it be actually better to kill that regex implementation as well, and use the class provided by LLVMSupport everywhere? I started with the other since it caused direct build issues on Linux but I think I could try to kill this one as well.

It makes sense.

ki.stfu requested changes to this revision.Sep 11 2016, 10:37 PM
ki.stfu added a reviewer: ki.stfu.

You forgot to remove its header file

This revision now requires changes to proceed.Sep 11 2016, 10:37 PM
mgorny updated this revision to Diff 70997.Sep 12 2016, 5:32 AM
mgorny edited edge metadata.

You forgot to remove its header file

Thanks for noticing. Fixed now.

labath edited edge metadata.Sep 12 2016, 5:47 AM

@mgorny: The change should be reviewed by an lldb-mi maintainer (i.e., @ki.stfu). It helps is you explicitly specify the reviewer, as otherwise the person may not notice the patch.

It looks good. Remaining TODO for standalone builds:

  • sanitize six.py usage (it's installed into system-wide directory, where standard py-six lands)

How would you prefer handling it? Can we just kill it and rely on user installing it system-wide or via pip?

I think the most politically passable version would be to make that a cmake option (LLDB_USE_BUILTIN_SIX ?). If it is set you use the system-wide package, otherwise, you use the builtin one. When you build the distro package, you set it to false. When e.g., we are distributing it with Android Studio, we'll set it to true. I personally don't care what the default will be.

  • bump cmake_minimum_required(VERSION 2.8.12.2) to 3.4.3 in cmake/modules/LLDBStandalone.cmake Final bits for Windows:
  • eliminate the usage of ../lib/Support/regex_impl.h in RegularExpression.h

Wouldn't it be actually better to kill that regex implementation as well, and use the class provided by LLVMSupport everywhere? I started with the other since it caused direct build issues on Linux but I think I could try to kill this one as well.

I think that would be great. We'll need to be a bit careful and check whether we the two libraries have sufficiently similar interfaces.

ki.stfu accepted this revision.Sep 12 2016, 11:50 AM
ki.stfu edited edge metadata.

lgtm

This revision is now accepted and ready to land.Sep 12 2016, 11:50 AM
labath closed this revision.Sep 13 2016, 3:48 AM

I am going to assume you still want me to submit this. r281317.

Yes, thanks a lot.