Page MenuHomePhabricator

eliminate nontrivial Reset(...) from TypedPythonObject
ClosedPublic

Authored by lawrence_danna on Oct 17 2019, 12:49 PM.

Details

Summary

This deletes Reset(...), except for the no-argument form Reset()
from TypedPythonObject, and therefore from PythonString, PythonList,
etc.

It updates the various callers to use assignment, As<>, Take<>,
and Retain<>, as appropriate.

followon to https://reviews.llvm.org/D69080

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 12:49 PM

I think the conversion of different string types should be handled differently. Please see inline comment for details. Otherwise, this looks fine.

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
350–360

This is the annoying part about interacting with external "c" apis in llvm. Llvm really likes to have StringRefs everywhere, but since they're not guaranteed to be null-terminated, it usually ends up requiring a string copy (.str().c_str()).
Having the wrapper APIs take const char * is not really a good option because that pushes the problem down to its users (and/or creates redundant strlen()s everywhere), whereas the purpose of these APIs should be to create an interface that interacts nicely with the rest of the codebase.

There are two ways to get around that, depending on how much you want to optimize stuff:

  1. Just have your functions take a StringRef and call .str().c_str() when you need a null-terminated version of it
  2. Have your functions take a llvm::Twine. This class has a method https://llvm.org/doxygen/classllvm_1_1Twine.html#a5d968bc337af0be1f18813553a046df6, which enables you to avoid a string copy in the cases where the user passed in a null-terminated string, while avoiding to define multiple overloads of all your functions.

It is used as such:

auto my_foo_wrapper(const llvm::Twine &str) {
  llvm::SmallString<32 /*or whatever is the "typical" size*/> storage;
  llvm::StringRef s = str.toNullTerminatedStringRef(storage); // s is either the original const char *, or points to "storage"
  return foo(s.data() /*null terminated*/);
}

For this code I'd be fine with either option, but I'd like to avoid having two overloads of all functions (particularly if none of them is a StringRef).

422–424

Is this needed, given the "using" declaration above?

427

Isn't this already set by the superclass constructor?

735–743

Could you put this into the "python" namespace, since we already have it? Otherwise, it looks like a generic lldb_private function (which I would very much like to avoid it becoming).

lawrence_danna marked 6 inline comments as done.

fixed

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
422–424

yes, it seems when you delete one override of a member function, it deletes all the other ones that you don't explicitly mention too. Actually it's the using line that should be deleted.

427

yes, fixed

Harbormaster completed remote builds in B39806: Diff 225687.

use enable_if on CStringArg so the other constructors take precedence

While I appreciate the desire to reduce "boilerplate", I also think there's a lot of value in consistency. None of the existing APIs in llvm do anything like this, and are perfectly happy to call toNullTerminatedStringRef manually. You've now introduced another string-like type, which people have to figure out how it works. If you really feel like you must avoid the extra line of code (because that's the only thing that this saves, really) then you could at least move the class into the implementation of the methods, so that at least the inferface is consistent with all other llvm functions taking an llvm::Twine. You could define an llvm::Twine constructor and a "const char *" conversion operator for this class, so that you can type Py_whatever(NullTerminated(twine)). This way we could also define a "char *" conversion operator on the class in the PY2 case, also avoiding the need for the py2_const_cast thingy.

While I appreciate the desire to reduce "boilerplate", I also think there's a lot of value in consistency....

oh, yea somehow I thought going through the twine would still copy the string, I see you're right, it won't.

labath accepted this revision.Oct 19 2019, 1:54 AM
This revision is now accepted and ready to land.Oct 19 2019, 1:54 AM
labath added inline comments.Oct 19 2019, 2:05 AM
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
154

Actually, could you also write a short blurb about what is the purpose of this class.

comment on NullTerminated

lawrence_danna marked an inline comment as done.Oct 19 2019, 10:19 AM
This revision was automatically updated to reflect the committed changes.