labath added a comment.

Yes, that's the API I had in mind, but check out the inline comments for some 
problems/improvements.



================
Comment at: lldb/source/Host/common/XML.cpp:141
+      attr_value = (const char *)value;
+      free(value);
+    }
----------------
this should be xmlFree, per the function documentation.


================
Comment at: lldb/source/Host/common/XML.cpp:157-159
+  std::string attr_str = GetAttributeValue(name, "");
+  llvm::StringRef attr(attr_str);
+  return llvm::to_integer(attr, value, base);
----------------
I don't see why we need these temporary variables. std::string is implicitly 
convertible to a StringRef, and you don't need the value afterwards, so you 
should be able to pass the GetAttributeValue straight into the to_integer 
function. Did that not work for some reason?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4534
+    std::string main_lm_str = root_element.GetAttributeValue("main-lm");
+    llvm::StringRef main_lm(main_lm_str);
     // FIXME: we're silently ignoring invalid data here
----------------
Same here. `.empty()` and `to_integer` should work on std::string as well. 
Constructing a StringRef might make sense if we needed to perform some more 
complex operations (ones which std::string does not support) but I don't see 
anything like that here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116707

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to