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

[Tamas Berghammer via Phabricator via lldb-commits]

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
+private:
+  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.


https://reviews.llvm.org/D32930