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

[Jason Majors via Phabricator via lldb-commits]

jmajors marked 5 inline comments as done.
jmajors added inline comments.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:200
+
+  return std::stoul(big_endian, nullptr, 16);
+}
----------------
krytarowski wrote:
> Throws exceptions.
I didn't change this, because I'm expecting to remove the whole function. 


================
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);
+      }
----------------
zturner wrote:
> 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?
It doesn't matter when I do it. The issue with the other functions was they were converting target to host, when both were little. For string parsing, it needs target to big to string, or target to string to big.


================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:76-80
+  for (size_t i = 0; i < inferior_args.size(); i++) {
+    if (i > 0) command << ',';
+    string hex_encoded = HexEncode(inferior_args[i]);
+    command << hex_encoded.size() << ',' << i << ',' << hex_encoded;
+  }
----------------
zturner wrote:
> `for (const auto &arg : inferior_args)`
I need an parameter index to pass to the command, so the classic for loop makes more sense.


https://reviews.llvm.org/D32930