[PATCH] extracting swapStruct into include/llvm/Support/MachO.h (no functional change)

[Duncan P. N. Exon Smith]

> On 2014-Jul-04, at 01:26, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:
> 
> Hello Duncan,
> 
> Ping regarding that proposed change.
> 
> Also updating the email subject to be a bit more descriptive.
> 
> 
> -----Original Message-----
> From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
> Sent: 23 June 2014 15:37
> To: 'Duncan P. N. Exon Smith'
> Cc: llvm-commits
> Subject: RE: [PATCH] Some code improvements (no functional change)
> 
> Thank you Duncan!
> Committed as r210973--r210980, except for the namespace-related changes that
> you pointed out.
> 
> While committing this, I realised that the instantiations of swapStruct() in
> tools/lld/lib/ReaderWriter/MachO/MachONormalizedFileBinaryUtils.h are
> identical to the private implementations in lib/Object/MachOObjectFile.cpp
> 
> Should perhaps these swapStruct()'s be extracted into
> include/llvm/Support/MachO.h ?
> Attaching the patch for review.

Sorry, I've lost context on this -- can you re-attach the patch?

> 
> 
>>> 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.
> 
> Indeed this wasn't related to extracting SwapValue out of that file; but is
> this cleanup any controversial?
> 
> 
>>> 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.
>> 
>>> @@ -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.
> 
> The only reason for that change was to be consistent with the other file.
> Same as yourself, I'm not a big fan of "using" statements :-)
> 
> Is it OK to commit these two namespace changes?

Can't remember if I replied to this part already.  Coding standards say
that `using` clauses should be used in source files when (and only when)
implementing parts of that namespace [1].  Relevant text quoted here:

> The exception to the general rule (i.e. it’s not an exception for the
> std namespace) is for implementation files. For example, all of the code
> in the LLVM project implements code that lives in the ‘llvm’ namespace.
> As such, it is ok, and actually clearer, for the .cpp files to have a
> 'using namespace llvm;' directive at the top, after the #includes. This
> reduces indentation in the body of the file for source editors that
> indent based on braces, and keeps the conceptual context cleaner. The
> general form of this rule is that any .cpp file that implements code in
> any namespace may use that namespace (and its parents’), but should not
> use any others.

So I think `MachOUniversal.cpp` is correct as is.  Personally, I prefer
reading/writing `using` clauses for nested namespaces as:

    using namespace llvm;
    using namespace llvm::object;

http://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std