[objcopy][NFC] Add doc comments to the executeObjcopy* functions.

[avl]

Summary

Depends on D88827

[jhenderson]

llvm/include/llvm/Object/ObjCopy/COFF/COFFObjcopy.h

27 ↗

(On Diff #389144)

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

27 ↗

(On Diff #389144)

34 ↗

(On Diff #389144)

"Binary" format isn't particularly descriptive, since it's more that objcopy should just wrap the data in a section and add some symbols. Technically all files on disk are "binary" format, since they are written in binary!

41 ↗

(On Diff #389144)

I'm a little thrown by the "Unspecified" format. I'm guessing that's just because earlier logic will detect that the input buffer is an ELF file if no explicit input format is specified, and feed it through to here. If that's the case, we shouldn't be mentioning this case, since in both cases, we're working with an ELF file.

llvm/include/llvm/Object/ObjCopy/MachO/MachOObjcopy.h

28 ↗

(On Diff #389144)

I'm assuming this is correct, i.e. that the input binary must be Mach-O to get here?

35 ↗

(On Diff #389144)

llvm/include/llvm/Object/ObjCopy/wasm/WasmObjcopy.h

26 ↗

(On Diff #389144)

[avl]

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

41 ↗

(On Diff #389144)

The idea of these descriptions is to match with values which are in:

struct CommonConfig {
...
  FileFormat InputFormat = FileFormat::Unspecified;
...
};

enum class FileFormat {
  Unspecified,
  ELF,
  Binary,
  IHex,
};

It would probably be better to make FileFormat more descriptive by adding values: COFF, MachO, Wasm instead of Unspecified; RawELF instead of Binary... But I think this refactoring should be done separately. The idea of current doc comments is to show connection of CommonConfig.InputFormat and corresponding executeObjcopyOn* functions.

[avl]

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

41 ↗

(On Diff #389144)

Current logic is that Unspecified is used for all formats:

// FIXME:  Currently, we ignore the target for non-binary/ihex formats
// explicitly specified by -I option (e.g. -Ielf32-x86-64) and guess the
// format by llvm::object::createBinary regardless of the option value.
Config.InputFormat = StringSwitch<FileFormat>(InputFormat)
                         .Case("binary", FileFormat::Binary)
                         .Case("ihex", FileFormat::IHex)
                         .Default(FileFormat::Unspecified);

So, How about that variant:

/// Applies the transformations described by \p Config and \p ELFConfig to
/// \p In, which must be an ELF object(FileFormat::Unspecified), and writes
/// the result into \p Out.

?

[jhenderson]

llvm/include/llvm/Object/ObjCopy/COFF/COFFObjcopy.h

27 ↗

(On Diff #389144)

See below: actually just delete the bit in brackets, so it's "... \p In and writes...".

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

41 ↗

(On Diff #389144)

The FileFormat specified in the Config is completely irrelevant in these functions - a future piece of code may not use the FileFormat, so mentioning it in the comments is confusing at best, and at worst directly misleading.

What matters is what the code in the function expects the format of the input to be. In fact, I realised in this case that we are already passing in an ELFObjectFileBase for In, so we probably don't need to say anything specifically about In at all, i.e. the comment should be "... to \p In and writes ..."

llvm/include/llvm/Object/ObjCopy/MachO/MachOObjcopy.h

28 ↗

(On Diff #389144)

Scratch this: just delete the the bit between "In" and the "and writes". The type is already defined by the type of In.

35 ↗

(On Diff #389144)

Same as above: delete the bit after "In" up to (but not including) "and writes".

llvm/include/llvm/Object/ObjCopy/wasm/WasmObjcopy.h

26 ↗

(On Diff #389144)

As above, just delete the bit between "In" and "and writes".

[avl]

addressed comments.

[jhenderson]

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

27 ↗

(On Diff #389144)

In this and the next case, you probably still want the full inline suggestion, as the format isn't defined by the input type (a MemoryBuffer doesn't know whether it's to be treated as IHex or raw binary, or something else entirely).

[avl]

addressed comments.

[jhenderson]

Please use my inline suggestions, not your original text, as requested in the comment already...

[avl]

addressed comments.

[jhenderson]

Thanks, LGTM.

[avl]

Thank you for the review.

[MaskRay]

llvm/include/llvm/Object/ObjCopy/ELF/ELFObjcopy.h

26 ↗

(On Diff #390005)

I think imperative sentences are more common: Applies => Apply

[avl]

rebased.

llvm/include/llvm/ObjCopy/COFF/COFFObjcopy.h

 } // end namespace object
 
 namespace objcopy {
 struct CommonConfig;
 struct COFFConfig;
 
 namespace coff {
 
+/// Apply the transformations described by \p Config and \p COFFConfig
+/// to \p In and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnBinary(const CommonConfig &Config, const COFFConfig &,
                              object::COFFObjectFile &In, raw_ostream &Out);
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_COFF_COFFOBJCOPY_H

llvm/include/llvm/ObjCopy/ELF/ELFObjcopy.h

 class ELFObjectFileBase;
 } // end namespace object
 
 namespace objcopy {
 struct CommonConfig;
 struct ELFConfig;
 
 namespace elf {
+/// Apply the transformations described by \p Config and \p ELFConfig to
+/// \p In, which must represent an IHex file, and writes the result
+/// into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnIHex(const CommonConfig &Config,
                            const ELFConfig &ELFConfig, MemoryBuffer &In,
                            raw_ostream &Out);
+
+/// Apply the transformations described by \p Config and \p ELFConfig to
+/// \p In, which is treated as a raw binary input, and writes the result
+/// into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnRawBinary(const CommonConfig &Config,
                                 const ELFConfig &ELFConfig, MemoryBuffer &In,
                                 raw_ostream &Out);
+
+/// Apply the transformations described by \p Config and \p ELFConfig to
+/// \p In and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnBinary(const CommonConfig &Config,
                              const ELFConfig &ELFConfig,
                              object::ELFObjectFileBase &In, raw_ostream &Out);
 
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_ELF_ELFOBJCOPY_H

llvm/include/llvm/ObjCopy/MachO/MachOObjcopy.h

 } // end namespace object
 
 namespace objcopy {
 struct CommonConfig;
 struct MachOConfig;
 class MultiFormatConfig;
 
 namespace macho {
+/// Apply the transformations described by \p Config and \p MachOConfig to
+/// \p In and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnBinary(const CommonConfig &Config,
                              const MachOConfig &MachOConfig,
                              object::MachOObjectFile &In, raw_ostream &Out);
 
+/// Apply the transformations described by \p Config and \p MachOConfig to
+/// \p In and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnMachOUniversalBinary(
     const MultiFormatConfig &Config, const object::MachOUniversalBinary &In,
     raw_ostream &Out);
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_MACHO_MACHOOBJCOPY_H

llvm/include/llvm/ObjCopy/wasm/WasmObjcopy.h

 class WasmObjectFile;
 } // end namespace object
 
 namespace objcopy {
 struct CommonConfig;
 struct WasmConfig;
 
 namespace wasm {
+/// Apply the transformations described by \p Config and \p WasmConfig
+/// to \p In and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
 Error executeObjcopyOnBinary(const CommonConfig &Config, const WasmConfig &,
                              object::WasmObjectFile &In, raw_ostream &Out);
 
 } // end namespace wasm
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_WASM_WASMOBJCOPY_H