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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
