[Lldb-commits] [PATCH] D67369: Implement DW_OP_convert

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Looks good overall, especially the testing.



================
Comment at: lldb/include/lldb/Utility/Scalar.h:107
+  /// Return the most efficient Scalar::Type for the requested size.
+  static Type GetBestType(size_t bit_size, bool sign);
+
----------------
JDevlieghere wrote:
> How about `GetTypeForBitSize`? 
nit: bool signed ?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2571
+      if (stack.size() < 1) {
+        if (error_ptr)
+          error_ptr->SetErrorString(
----------------
JDevlieghere wrote:
> aprantl wrote:
> > JDevlieghere wrote:
> > > Can we wrap this in a lambda? 
> > Which part specifically do you mean?
> ```auto SetErrorString = [&](llvm::StringRef msg) { if (error_ptr) error_ptr->SetErrorString(msg); })```
> 
> We could deduplicate even more code by using a macro that also covers the check around it and does the return, but I'm not sure if that's really worth it. 
Oh, like LLDB_LOG? Seems like a nice idea. I think I need this for D67376, so I might do it if no one else has started already. It'd be great to use that here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67369/new/

https://reviews.llvm.org/D67369