[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:
> > 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.
> 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.

See also md5_contents, is_local, make_absolute, copy_file, and various other methods in this file, and most of them have simple behaviour. Each of them have nearly-identical doxygen comments for multiple overloads. You may disagree with this, but that is the established pattern, and makes a lot more sense when you're reading the doxygen itself. You can probably get away with a single-line comment referring to the "main" overload (i.e. the one with the comments), just noting the different parameter.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D64236