Page MenuHomePhabricator

[lldb] [gdb-remote client] Refactor handling qSupported
ClosedPublic

Authored by mgorny on Apr 8 2021, 3:23 PM.

Details

Summary

Refactor the qSupported handler to split the reply into an array,
and identify features within the array rather than searching the string
for partial matches. While at it, use StringRef.split() to process
the compression list instead of reinventing the wheel.

Switch the arguments to MaybeEnableCompression() to use an ArrayRef
of StringRefs to simplify parameter passing from GetRemoteQSupported().

Diff Detail

Event Timeline

rovka accepted this revision.Apr 12 2021, 4:50 AM
rovka added a subscriber: rovka.

LGTM

This revision is now accepted and ready to land.Apr 12 2021, 4:50 AM
This revision was landed with ongoing or failed builds.Apr 12 2021, 3:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 3:23 PM

looks great, just fix the build errors :)

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
375

not a big deal, but this probably shouldn't be auto.

looks great, just fix the build errors :)

Yeah, I'm trying to see if I can reproduce them when building with Clang.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
375

Could you explain a bit? I thought auto is convenient here since the actual type is visible three lines higher.

looks great, just fix the build errors :)

Yeah, I'm trying to see if I can reproduce them when building with Clang.

I believe StringRef is not implicitly convertible to std::string these days, but all the conversions are in conditionally compiled code. Maybe you don't have any of it enabled?

My guess is it would be sufficient to change avail_name to a StringRef (and fix the errors caused by that).

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
375

Well.. my reasoning was something like this:

  • llvm guidelines say that auto should be used (only?) when the type is obvious from context
  • "context" is not exactly specified, but I would take that to mean the enclosing expression/statement (not the statement two lines up). The use of cast<Foo> in the example supports this.
  • the actual type is not that complicated (llvm::StringRef), so it e.g. won't cause the line to be split in two
  • given the above, and the generally cautious approach to auto in llvm, its better to err on the side of spelling out the type

But like I said, it's not a big deal...

mgorny marked 2 inline comments as done.Apr 13 2021, 2:19 AM
mgorny added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
375

Yeah, this makes sense.