[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

Tamas Berghammer via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu May 18 06:08:25 PDT 2017

tberghammer added inline comments.

Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:83
+// Common functions for parsing packet data.
+std::unordered_map<std::string, std::string> SplitPairList(const std::string& s);
+std::vector<std::string> SplitList(const std::string& s, char delimeter);
jmajors wrote:
> zturner wrote:
> > tberghammer wrote:
> > > What if the key isn't unique?
> > Return an `llvm::StringMap<StringRef>` here.  Also the argument should be a `StringRef`.
> I was working on the assumption that the keys in lldb response packets would be unique, and that it would map a key to a list of values if it needed to have a one:many relationship. Are there any that have duplicate keys?
I think in an lldb-server test we should make as little assumption about lldb-server as possible. I am not aware of any packet where duplicated keys are allowed but if we want to rely on it I think we should add a test expectation verifying this as having repeated keys would mean lldb-server is buggy (what is exactly what we want to catch here)

Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:41
+  std::string source;
The LLDB coding convention is to prefix member variables with "m_". Do we want to follow that one here or should we follow the LLVM one what is CapitalCase (you are following neither of them at the moment)?

Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:52
+  unsigned long ReadRegister(unsigned long register_id) const;
"unsigned long" is only 32 bit on some systems but a register can be arbitrary large so the return value should be something more generic. This is true for every location you are working with registers.

Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:63
+ public:
+  static llvm::Expected<std::unique_ptr<JThreadsInfo>> Create(
+      llvm::StringRef response, llvm::support::endianness endian);
Why do we need the unique_ptr here? Can it return llvm::Expected<ProcessInfo> instead? Same question for the other Create functions.


More information about the lldb-commits mailing list