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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: include/llvm/Support/FileSystem.h:670
+
+std::error_code setPermissions(int FD, perms Permissions);
 
----------------
jhenderson wrote:
> 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)?
> 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.

When the function does more involved tasks, I agree. Here, the function doesn't do anything smart. The comment just makes reading through the file harder.

> 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)?

chmod has much fewer use cases than fchmod. It is rare to chmod a foreign file. Usually, you also write/stat/read the file, then you'll need a handle. Once you have the handle, it is more natural to use fchmod (it also prevents TOCTOU race). setPermissions(Path, perms) was currently only used in one place in objcopy, I can hardly think of any future use case of it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64236