[PATCH] D115418: [dwarf][NFC] Move expandBundle() to MachO.h

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MachO.h:655-657
+  /// Returns true if the input path is a .dSYM bundle (as created by the
+  /// dsymutil tool) and populates `BinaryPaths` with paths to the object files
+  /// that are inside the bundle.
----------------
Probably need to distinguish when the fails with an error versus when it will return false, in the comments.


================
Comment at: llvm/include/llvm/Object/MachO.h:658-659
+  /// that are inside the bundle.
+  static Expected<bool> isDsymBundle(const StringRef Path,
+                                     std::vector<std::string> &BinaryPaths);
+
----------------
a) I wouldn't bother with `const` on the `StringRef`, since they're inately `const` anyway (unless you reassign them of course).
b) I don't think "is*" is a sensible name for a function that does some vector population. Perhaps "loadDsymBundle" or "findDsymObjectMembers" or something to that effect.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4725-4727
+Expected<bool>
+MachOObjectFile::isDsymBundle(const StringRef Path,
+                              std::vector<std::string> &BinaryPaths) {
----------------
clayborg wrote:
> My question here is: if an error is returned, will it make any sense to the caller? The only error returned here is if something goes wrong during directory iteration. Which could be a few things:
> - Path specifies a directory with ".dSYM" extension (do we are about case here?) and it doesn't the directory Path + "Context/Resources/DWARF" doesn't exist which would return something like "directory does not exist". Will it specify which directory doesn't exist? The user could specify "/path/to/foo.dSYM" that is a valid directory but it contains no DWARF bundle stuff inside. Just wondering if this will make sense to show to the user?
> - We can't get sys::fs::file_status on a file in the Path + "Context/Resources/DWARF" + File file which would return something like "permission denied" or something else. If the error doesn't contain the path that caused the problem the user will be confused.
> 
> And I wonder if the callers of this function should show those errors to the user and would it make any sense to the user? If the errors don't make sense, then it might be good to just return a "bool" instead of a "Expected<bool>", or just return a std::vector<std::string> that is empty or that contained any successful paths that were already extracted.
> 
> If we want to keep the errors we should make the error message more clear and include information about what path caused the problem with the EC error code appended. So for the first case where sys::fs::directory_iterator constructor fills in an error, maybe returning something like "unable to iterate over directory %s: %s" where the first %s is the Path and the second is the error string. Something similar for the second?
> 
> Let me know what your thoughts are.
I think we should return the error, with additional context (i.e. what path is bad etc), and allow client code to decide what to do with it: lower-level libraries shouldn't generally assume that clients don't care about when errors exist.

When issue that is worth thinking about: we could get into a situation where the code returns an error, but the vector has been modified. I'd at least mention that in the event of an error, the vector's content is indeterminate, but perhaps better should be: make a copy of the vector and replace it, in the event of an error after the main for loop has been started.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4740
+       Dir != DirEnd && !EC; Dir.increment(EC)) {
+    const std::string &Path = Dir->path();
+    sys::fs::file_status Status;
----------------



================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4751
+      break;
+    default: /*ignore*/;
+    }
----------------
This "ignore" is slightly surprising to me. Should we not emit an error of some kind for this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115418