[PATCH] Some code improvements (no functional change)
Duncan P. N. Exon Smith
dexonsmith at apple.com
Fri Jun 13 13:47:03 PDT 2014
> On 2014-Jun-13, at 03:59, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:
>
> Thank you all three for your comments!
>
> Attaching the revised patch for the change, as described in Duncan's previous mail, for your review:
>
> 1) SwapValue() which had been copypasted between lib/Object/MachOObjectFile.cpp and lib/Object/MachOUniversal.cpp now extracted into include/llvm/Support/SwapByteOrder.h and renamed into swapByteOrder()
> 2) Tests for swapByteOrder() added into unittests/Support/SwapByteOrderTest.cpp
> 3) The more common use-case of applying SwapByteOrder() in-place replaced with a call to swapByteOrder() in lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h, lib/MC/ELFObjectWriter.cpp, lib/Support/DataExtractor.cpp, include/llvm/ADT/Hashing.h, and include/llvm/Support/Endian.h
> 4) SwapByteOrder() renamed into getSwappedBytes(); the only two places where it had been used not in-place are lib/ProfileData/InstrProfReader.cpp and include/llvm/ProfileData/InstrProfReader.h (and its own unit test)
>
> OK to commit it like this?
This is mostly okay, but should be staged as three separate commits:
1. First patch renames `SwapByteOrder()` to `getSwappedBytes()`.
2. Second patch adds `swapByteOrder()` (and tests).
3. Third patch removes `SwapValue()` and replaces the uses with
`sys::swapByteOrder()`.
The cleanups to change from `SwapByteOrder()`/`getSwappedBytes()` to
`swapByteOrder()` are somewhat orthogonal and should be left out of
these patches. However, they're nice cleanups (and pretty obvious) --
feel free to commit them as follow-ups without pre-commit review.
I have some other low-level comments inline below.
> Index: lib/Object/MachOObjectFile.cpp
> ===================================================================
> --- lib/Object/MachOObjectFile.cpp (revision 210891)
> +++ lib/Object/MachOObjectFile.cpp (working copy)
> @@ -24,9 +24,6 @@
> #include <cstring>
> #include <limits>
>
> -using namespace llvm;
> -using namespace object;
> -
This cleanup seems unrelated. You should leave this out.
> namespace llvm {
>
> namespace object {
> @@ -43,201 +40,198 @@
> char segname[16];
> };
>
> -template<typename T>
> -static void SwapValue(T &Value) {
> - Value = sys::SwapByteOrder(Value);
> -}
> +using sys::swapByteOrder;
I'm not a big fan of having this `using` statement here. I'd rather you
used `sys::` explicitly for the uses that follow.
>
> template<typename T>
> static void SwapStruct(T &Value);
>
> template<>
> void SwapStruct(MachO::any_relocation_info &H) {
> - SwapValue(H.r_word0);
> - SwapValue(H.r_word1);
> + swapByteOrder(H.r_word0);
> + swapByteOrder(H.r_word1);
> }
>
> template<>
> void SwapStruct(MachO::load_command &L) {
> - SwapValue(L.cmd);
> - SwapValue(L.cmdsize);
> + swapByteOrder(L.cmd);
> + swapByteOrder(L.cmdsize);
> }
>
> template<>
> void SwapStruct(nlist_base &S) {
> - SwapValue(S.n_strx);
> - SwapValue(S.n_desc);
> + swapByteOrder(S.n_strx);
> + swapByteOrder(S.n_desc);
> }
>
> template<>
> void SwapStruct(MachO::section &S) {
> - SwapValue(S.addr);
> - SwapValue(S.size);
> - SwapValue(S.offset);
> - SwapValue(S.align);
> - SwapValue(S.reloff);
> - SwapValue(S.nreloc);
> - SwapValue(S.flags);
> - SwapValue(S.reserved1);
> - SwapValue(S.reserved2);
> + swapByteOrder(S.addr);
> + swapByteOrder(S.size);
> + swapByteOrder(S.offset);
> + swapByteOrder(S.align);
> + swapByteOrder(S.reloff);
> + swapByteOrder(S.nreloc);
> + swapByteOrder(S.flags);
> + swapByteOrder(S.reserved1);
> + swapByteOrder(S.reserved2);
> }
>
> template<>
> void SwapStruct(MachO::section_64 &S) {
> - SwapValue(S.addr);
> - SwapValue(S.size);
> - SwapValue(S.offset);
> - SwapValue(S.align);
> - SwapValue(S.reloff);
> - SwapValue(S.nreloc);
> - SwapValue(S.flags);
> - SwapValue(S.reserved1);
> - SwapValue(S.reserved2);
> - SwapValue(S.reserved3);
> + swapByteOrder(S.addr);
> + swapByteOrder(S.size);
> + swapByteOrder(S.offset);
> + swapByteOrder(S.align);
> + swapByteOrder(S.reloff);
> + swapByteOrder(S.nreloc);
> + swapByteOrder(S.flags);
> + swapByteOrder(S.reserved1);
> + swapByteOrder(S.reserved2);
> + swapByteOrder(S.reserved3);
> }
>
> template<>
> void SwapStruct(MachO::nlist &S) {
> - SwapValue(S.n_strx);
> - SwapValue(S.n_desc);
> - SwapValue(S.n_value);
> + swapByteOrder(S.n_strx);
> + swapByteOrder(S.n_desc);
> + swapByteOrder(S.n_value);
> }
>
> template<>
> void SwapStruct(MachO::nlist_64 &S) {
> - SwapValue(S.n_strx);
> - SwapValue(S.n_desc);
> - SwapValue(S.n_value);
> + swapByteOrder(S.n_strx);
> + swapByteOrder(S.n_desc);
> + swapByteOrder(S.n_value);
> }
>
> template<>
> void SwapStruct(MachO::mach_header &H) {
> - SwapValue(H.magic);
> - SwapValue(H.cputype);
> - SwapValue(H.cpusubtype);
> - SwapValue(H.filetype);
> - SwapValue(H.ncmds);
> - SwapValue(H.sizeofcmds);
> - SwapValue(H.flags);
> + swapByteOrder(H.magic);
> + swapByteOrder(H.cputype);
> + swapByteOrder(H.cpusubtype);
> + swapByteOrder(H.filetype);
> + swapByteOrder(H.ncmds);
> + swapByteOrder(H.sizeofcmds);
> + swapByteOrder(H.flags);
> }
>
> template<>
> void SwapStruct(MachO::mach_header_64 &H) {
> - SwapValue(H.magic);
> - SwapValue(H.cputype);
> - SwapValue(H.cpusubtype);
> - SwapValue(H.filetype);
> - SwapValue(H.ncmds);
> - SwapValue(H.sizeofcmds);
> - SwapValue(H.flags);
> - SwapValue(H.reserved);
> + swapByteOrder(H.magic);
> + swapByteOrder(H.cputype);
> + swapByteOrder(H.cpusubtype);
> + swapByteOrder(H.filetype);
> + swapByteOrder(H.ncmds);
> + swapByteOrder(H.sizeofcmds);
> + swapByteOrder(H.flags);
> + swapByteOrder(H.reserved);
> }
>
> template<>
> void SwapStruct(MachO::symtab_command &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.symoff);
> - SwapValue(C.nsyms);
> - SwapValue(C.stroff);
> - SwapValue(C.strsize);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.symoff);
> + swapByteOrder(C.nsyms);
> + swapByteOrder(C.stroff);
> + swapByteOrder(C.strsize);
> }
>
> template<>
> void SwapStruct(MachO::dysymtab_command &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.ilocalsym);
> - SwapValue(C.nlocalsym);
> - SwapValue(C.iextdefsym);
> - SwapValue(C.nextdefsym);
> - SwapValue(C.iundefsym);
> - SwapValue(C.nundefsym);
> - SwapValue(C.tocoff);
> - SwapValue(C.ntoc);
> - SwapValue(C.modtaboff);
> - SwapValue(C.nmodtab);
> - SwapValue(C.extrefsymoff);
> - SwapValue(C.nextrefsyms);
> - SwapValue(C.indirectsymoff);
> - SwapValue(C.nindirectsyms);
> - SwapValue(C.extreloff);
> - SwapValue(C.nextrel);
> - SwapValue(C.locreloff);
> - SwapValue(C.nlocrel);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.ilocalsym);
> + swapByteOrder(C.nlocalsym);
> + swapByteOrder(C.iextdefsym);
> + swapByteOrder(C.nextdefsym);
> + swapByteOrder(C.iundefsym);
> + swapByteOrder(C.nundefsym);
> + swapByteOrder(C.tocoff);
> + swapByteOrder(C.ntoc);
> + swapByteOrder(C.modtaboff);
> + swapByteOrder(C.nmodtab);
> + swapByteOrder(C.extrefsymoff);
> + swapByteOrder(C.nextrefsyms);
> + swapByteOrder(C.indirectsymoff);
> + swapByteOrder(C.nindirectsyms);
> + swapByteOrder(C.extreloff);
> + swapByteOrder(C.nextrel);
> + swapByteOrder(C.locreloff);
> + swapByteOrder(C.nlocrel);
> }
>
> template<>
> void SwapStruct(MachO::linkedit_data_command &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.dataoff);
> - SwapValue(C.datasize);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.dataoff);
> + swapByteOrder(C.datasize);
> }
>
> template<>
> void SwapStruct(MachO::segment_command &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.vmaddr);
> - SwapValue(C.vmsize);
> - SwapValue(C.fileoff);
> - SwapValue(C.filesize);
> - SwapValue(C.maxprot);
> - SwapValue(C.initprot);
> - SwapValue(C.nsects);
> - SwapValue(C.flags);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.vmaddr);
> + swapByteOrder(C.vmsize);
> + swapByteOrder(C.fileoff);
> + swapByteOrder(C.filesize);
> + swapByteOrder(C.maxprot);
> + swapByteOrder(C.initprot);
> + swapByteOrder(C.nsects);
> + swapByteOrder(C.flags);
> }
>
> template<>
> void SwapStruct(MachO::segment_command_64 &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.vmaddr);
> - SwapValue(C.vmsize);
> - SwapValue(C.fileoff);
> - SwapValue(C.filesize);
> - SwapValue(C.maxprot);
> - SwapValue(C.initprot);
> - SwapValue(C.nsects);
> - SwapValue(C.flags);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.vmaddr);
> + swapByteOrder(C.vmsize);
> + swapByteOrder(C.fileoff);
> + swapByteOrder(C.filesize);
> + swapByteOrder(C.maxprot);
> + swapByteOrder(C.initprot);
> + swapByteOrder(C.nsects);
> + swapByteOrder(C.flags);
> }
>
> template<>
> void SwapStruct(uint32_t &C) {
> - SwapValue(C);
> + swapByteOrder(C);
> }
>
> template<>
> void SwapStruct(MachO::linker_options_command &C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.count);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.count);
> }
>
> template<>
> void SwapStruct(MachO::version_min_command&C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.version);
> - SwapValue(C.reserved);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.version);
> + swapByteOrder(C.reserved);
> }
>
> template<>
> void SwapStruct(MachO::dylib_command&C) {
> - SwapValue(C.cmd);
> - SwapValue(C.cmdsize);
> - SwapValue(C.dylib.name);
> - SwapValue(C.dylib.timestamp);
> - SwapValue(C.dylib.current_version);
> - SwapValue(C.dylib.compatibility_version);
> + swapByteOrder(C.cmd);
> + swapByteOrder(C.cmdsize);
> + swapByteOrder(C.dylib.name);
> + swapByteOrder(C.dylib.timestamp);
> + swapByteOrder(C.dylib.current_version);
> + swapByteOrder(C.dylib.compatibility_version);
> }
>
> template<>
> void SwapStruct(MachO::data_in_code_entry &C) {
> - SwapValue(C.offset);
> - SwapValue(C.length);
> - SwapValue(C.kind);
> + swapByteOrder(C.offset);
> + swapByteOrder(C.length);
> + swapByteOrder(C.kind);
> }
>
> template<typename T>
> Index: lib/Object/MachOUniversal.cpp
> ===================================================================
> --- lib/Object/MachOUniversal.cpp (revision 210891)
> +++ lib/Object/MachOUniversal.cpp (working copy)
> @@ -19,30 +19,26 @@
> #include "llvm/Support/Host.h"
> #include "llvm/Support/MemoryBuffer.h"
>
> -using namespace llvm;
> -using namespace object;
> +namespace llvm {
> +namespace object {
I don't see a reason to change the namespace setup, but even if it's a
good idea it shouldn't be part of this commit.
> +using sys::swapByteOrder;
Same comment about `using`. Leave it out and use `sys::swapByteOrder()`
below.
>
> template<typename T>
> -static void SwapValue(T &Value) {
> - Value = sys::SwapByteOrder(Value);
> -}
> -
> -template<typename T>
> static void SwapStruct(T &Value);
>
> template<>
> void SwapStruct(MachO::fat_header &H) {
> - SwapValue(H.magic);
> - SwapValue(H.nfat_arch);
> + swapByteOrder(H.magic);
> + swapByteOrder(H.nfat_arch);
> }
>
> template<>
> void SwapStruct(MachO::fat_arch &H) {
> - SwapValue(H.cputype);
> - SwapValue(H.cpusubtype);
> - SwapValue(H.offset);
> - SwapValue(H.size);
> - SwapValue(H.align);
> + swapByteOrder(H.cputype);
> + swapByteOrder(H.cpusubtype);
> + swapByteOrder(H.offset);
> + swapByteOrder(H.size);
> + swapByteOrder(H.align);
> }
>
> template<typename T>
> @@ -165,3 +161,6 @@
> }
> return object_error::arch_not_found;
> }
> +
> +} // end namespace object
> +} // end namespace llvm
This is part of the unrelated change above; remove it too.
> Index: lib/Support/DataExtractor.cpp
> ===================================================================
> --- lib/Support/DataExtractor.cpp (revision 210891)
> +++ lib/Support/DataExtractor.cpp (working copy)
> @@ -21,7 +21,7 @@
> if (de->isValidOffsetForDataOfSize(offset, sizeof(val))) {
> std::memcpy(&val, &Data[offset], sizeof(val));
> if (sys::IsLittleEndianHost != isLittleEndian)
> - val = sys::SwapByteOrder(val);
> + sys::swapByteOrder(val);
>
> // Advance the offset
> *offset_ptr += sizeof(val);
> Index: include/llvm/ProfileData/InstrProfReader.h
> ===================================================================
> --- include/llvm/ProfileData/InstrProfReader.h (revision 210891)
> +++ include/llvm/ProfileData/InstrProfReader.h (working copy)
> @@ -184,7 +184,7 @@
> std::error_code readHeader(const RawHeader &Header);
> template <class IntT>
> IntT swap(IntT Int) const {
> - return ShouldSwapBytes ? sys::SwapByteOrder(Int) : Int;
> + return ShouldSwapBytes ? sys::getSwappedBytes(Int) : Int;
> }
> const uint64_t *getCounter(IntPtrT CounterPtr) const {
> ptrdiff_t Offset = (swap(CounterPtr) - CountersDelta) / sizeof(uint64_t);
> Index: include/llvm/ADT/Hashing.h
> ===================================================================
> --- include/llvm/ADT/Hashing.h (revision 210891)
> +++ include/llvm/ADT/Hashing.h (working copy)
> @@ -152,7 +152,7 @@
> uint64_t result;
> memcpy(&result, p, sizeof(result));
> if (sys::IsBigEndianHost)
> - return sys::SwapByteOrder(result);
> + sys::swapByteOrder(result);
> return result;
> }
>
> @@ -160,7 +160,7 @@
> uint32_t result;
> memcpy(&result, p, sizeof(result));
> if (sys::IsBigEndianHost)
> - return sys::SwapByteOrder(result);
> + sys::swapByteOrder(result);
> return result;
> }
>
> Index: include/llvm/Support/Endian.h
> ===================================================================
> --- include/llvm/Support/Endian.h (revision 210891)
> +++ include/llvm/Support/Endian.h (working copy)
> @@ -38,7 +38,7 @@
> template<typename value_type, endianness endian>
> inline value_type byte_swap(value_type value) {
> if (endian != native && sys::IsBigEndianHost != (endian == big))
> - return sys::SwapByteOrder(value);
> + sys::swapByteOrder(value);
> return value;
> }
>
> Index: include/llvm/Support/SwapByteOrder.h
> ===================================================================
> --- include/llvm/Support/SwapByteOrder.h (revision 210891)
> +++ include/llvm/Support/SwapByteOrder.h (working copy)
> @@ -68,33 +68,38 @@
> #endif
> }
>
> -inline unsigned char SwapByteOrder(unsigned char C) { return C; }
> -inline signed char SwapByteOrder(signed char C) { return C; }
> -inline char SwapByteOrder(char C) { return C; }
> +inline unsigned char getSwappedBytes(unsigned char C) { return C; }
> +inline signed char getSwappedBytes(signed char C) { return C; }
> +inline char getSwappedBytes(char C) { return C; }
>
> -inline unsigned short SwapByteOrder(unsigned short C) { return SwapByteOrder_16(C); }
> -inline signed short SwapByteOrder( signed short C) { return SwapByteOrder_16(C); }
> +inline unsigned short getSwappedBytes(unsigned short C) { return SwapByteOrder_16(C); }
> +inline signed short getSwappedBytes( signed short C) { return SwapByteOrder_16(C); }
>
> -inline unsigned int SwapByteOrder(unsigned int C) { return SwapByteOrder_32(C); }
> -inline signed int SwapByteOrder( signed int C) { return SwapByteOrder_32(C); }
> +inline unsigned int getSwappedBytes(unsigned int C) { return SwapByteOrder_32(C); }
> +inline signed int getSwappedBytes( signed int C) { return SwapByteOrder_32(C); }
>
> #if __LONG_MAX__ == __INT_MAX__
> -inline unsigned long SwapByteOrder(unsigned long C) { return SwapByteOrder_32(C); }
> -inline signed long SwapByteOrder( signed long C) { return SwapByteOrder_32(C); }
> +inline unsigned long getSwappedBytes(unsigned long C) { return SwapByteOrder_32(C); }
> +inline signed long getSwappedBytes( signed long C) { return SwapByteOrder_32(C); }
> #elif __LONG_MAX__ == __LONG_LONG_MAX__
> -inline unsigned long SwapByteOrder(unsigned long C) { return SwapByteOrder_64(C); }
> -inline signed long SwapByteOrder( signed long C) { return SwapByteOrder_64(C); }
> +inline unsigned long getSwappedBytes(unsigned long C) { return SwapByteOrder_64(C); }
> +inline signed long getSwappedBytes( signed long C) { return SwapByteOrder_64(C); }
> #else
> #error "Unknown long size!"
> #endif
>
> -inline unsigned long long SwapByteOrder(unsigned long long C) {
> +inline unsigned long long getSwappedBytes(unsigned long long C) {
> return SwapByteOrder_64(C);
> }
> -inline signed long long SwapByteOrder(signed long long C) {
> +inline signed long long getSwappedBytes(signed long long C) {
> return SwapByteOrder_64(C);
> }
>
> +template<typename T>
> +static void swapByteOrder(T &Value) {
This shouldn't be `static`. However, it should be `inline`.
> + Value = getSwappedBytes(Value);
> +}
> +
> } // end namespace sys
> } // end namespace llvm
More information about the llvm-commits
mailing list