[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.


================
Comment at: lldb/source/API/SBTarget.cpp:1589
 
-  const ConstString csFrom(from), csTo(to);
-  if (!csFrom)
+  llvm::StringRef srFrom(from), srTo(to);
+  if (srFrom.empty())
----------------
Aside (since the old code did this too): Generally code should prefer `=` initialization over `()` initialization - because the former can't invoke explicit conversions, so is easier to read because it limits the possible conversion to simpler/safer ones (for instance, with unique ptrs "std::unique_ptr<int> p = x();" tells me `x()` returns a unique_ptr or equivalent, but `std::unique_ptr<int> p(x());` could be that `x()` returns a raw pointer and then I have to wonder if it meant to transfer ownership or not - but I don't have to wonder that in the first example because the type system checks that for me).

Admittedly StringRef has no explicit ctors - but it's a good general style to use imho (would be happy to include this in the LLVM style guide if this seems contentious in any way & is worth some discussion/formalization - but I think it's just generally good C++ practice & hopefully can be accepted as such)


================
Comment at: lldb/source/Target/PathMappingList.cpp:35
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string and turn it back into a ConstString.
+  return FileSpec(path).GetPath();
----------------
out of date comment - maybe this comment isn't pulling its weight? The content seems fairly simple/probably self explanatory?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863