[Lldb-commits] [PATCH] D88632: A handful of fixes to lldb's "scan the local filesystem for kexts & kernels" feature

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I don't know much about this code so I've left some inline nits.



================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:505
+  bool is_kernel_filename =
+      file_spec.GetFilename().GetStringRef().startswith("kernel") ||
+      file_spec.GetFilename().GetStringRef().startswith("mach");
----------------
Nit: You could assign the StringRef to a temporary so you don't have to repeat `file_spec.GetFilename().GetStringRef()` thrice. 


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:680
+  std::string possible_path = kernel_dsym.GetPath();
+  if (!kernel_dsym.GetFilename().GetStringRef().endswith(".dSYM"))
+    return false;
----------------
`FileSpec` has a `GetFileNameExtension` but this works too. 


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:684-685
+  FileSpec binary_filespec = kernel_dsym;
+  std::string filename = binary_filespec.GetFilename().GetCString();
+  filename.erase(filename.size() - 5, filename.size()); // chop off '.dSYM'
+  binary_filespec.GetFilename() = ConstString(filename);
----------------
`FileSpec::GetFileNameStrippingExtension` 


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:716
+  // get rid of '.dSYM'
+  filename.erase(filename.size() - 5, filename.size());
+
----------------
`FileSpec::GetFileNameStrippingExtension` 


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:865
+        } else {
+          error = ModuleList::GetSharedModule(kern_spec, module_sp, NULL, NULL,
+                                              NULL);
----------------
Huh, how did we end up with `NULL` again, someone ran a clang tidy check over lldb that replaces it with `nullptr`. Do we only build this on Darwin maybe? If the rest of the file uses `NULL` we can fix that inconsistency in a separate commit. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88632