[PATCH] D64236: [llvm-objcopy] Don't change permissions of non-regular output files

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Support/FileSystem.h:670
+
+std::error_code setPermissions(int FD, perms Permissions);
 
----------------
MaskRay wrote:
> jhenderson wrote:
> > Doxygen comments to match the other overload?
> The purpose is obvious from the function signature and the overload above. Adding another few lines of comment just clutter the code.
> 
> Actually, I think `std::error_code setPermissions(const Twine &Path, perms Permissions);` should just be removed if we can find an implementation of fchmod on Windows - it is very rare to set mode bits of an unknown file (with a path).
In the source code, I agree, but if you don't add the doxygen comments, it will have none in the doxygen output, and therefore will look odd and is not so easy to switch around. Also, they have clearly different signatures so the meaning could be considered different.

Also, why would we delete the path overload? What if we don't have easy access to a FD (or don't want to try to look one up)?


================
Comment at: lib/Support/Windows/Path.inc:778
+  // FIXME Not implemented.
+  return std::error_code(ENOTSUP, std::generic_category());
+}
----------------
`make_error_code(errc::not_supported)` would be a bit cleaner.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64236