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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+  LLVM_BUILTIN_DEBUGTRAP;
+  delay = false;
----------------
This will work on MSVC and presumably clang.  I'm not sure about gcc.  Is that sufficient for your needs?   Do you know if gcc has the `__builtin_debugtrap` intrinsic?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:83-88
+      if (endian == LITTLE) {
+        registers[register_id] = SwitchEndian(value_str);
+      }
+      else {
+        registers[register_id] = stoul(value_str, nullptr, 16);
+      }
----------------
jmajors wrote:
> zturner wrote:
> > This code block is unnecessary.
> > 
> > ```
> > unsigned long X;
> > if (!value_str.getAsInteger(X))
> >   return some error;
> > llvm::support::endian::write(&registers[register_id], X, endian);
> > ```
> > 
> > By using llvm endianness functions you can just delete the `SwitchEndian()` function entirely, as it's not needed.
> These endian functions don't work here. getAsInteger() assumes the number is in human reading order (i.e. big endian). So it's converting the little endian string as if it's big, then converting that little endian long to another little endian long, which does nothing.
> I need something that reverses the string first.
> 
> What do you think about adding a version of StringRef::getAsInteger that accepts an endianness parameter?
Hmm..  What about this?

```
unsigned long X;
if (!value_str.getAsInteger(X))
  return some error;
sys::swapByteOrder(X);
```

It shouldn't matter if you swap the bytes in the string before doing the translation, or swap the bytes in the number after doing the translation should it?


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:64
+public:
+  static std::shared_ptr<StopReply> Create(const llvm::StringRef response,
+                                           llvm::support::endianness endian);
----------------
Can you constructor argument does not need to be `const`, as `StringRefs` are immutable by definition.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:69
+protected:
+  explicit StopReply();
+
----------------
Don't need `explicit` if there are no arguments.  It's mostly useful for single argument constructors.


https://reviews.llvm.org/D32930