[PATCH] D68772: [COFF] Wrap things in namespace lld { namespace coff {

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

> Similar to D67323 <https://reviews.llvm.org/D67323>

Is that the right reference? @MaskRay may I ask what was the initial concern?



================
Comment at: lld/COFF/DebugTypes.cpp:213-214
 // moved here.
-Expected<llvm::pdb::NativeSession *>
-lld::coff::findTypeServerSource(const ObjFile *f) {
   Expected<TypeServerSource *> ts = TypeServerSource::findFromFile(f);
----------------
MaskRay wrote:
> MaskRay wrote:
> > rnk wrote:
> > > MaskRay wrote:
> > > > rnk wrote:
> > > > > I prefer this style for free functions because it makes it a hard error if there's a mismatch between the header and the cpp file. It's a pretty simple style rule: every function implemented in a .cpp file should either be qualified with a class or namespace name, or it should be marked static. Then you never have to worry about what the active namespace is outside of headers.
> > > > > 
> > > > > That's just my personal preference and it's not in CodingStandards, but given how much we use free functions in LLD and LLVM, it's kind of nice.
> > > > Does the argument mean this patch should be reverted?
> > > > 
> > > > If we have interleaved classes and free functions, we may have:
> > > > 
> > > > ```
> > > > namespace lld {
> > > > namespace coff {
> > > > void Class::method0() {}
> > > > }
> > > > }
> > > > 
> > > > void lld::coff::free0() {} // we have to leave the active namespace, because otherwise [-Wextra-qualification]
> > > > 
> > > > namespace lld {
> > > > namespace coff {
> > > > void Class::method1() {}
> > > > }
> > > > }
> > > > 
> > > > void lld::coff::free1() {}
> > > > ```
> > > > 
> > > > Instead of doing that, this patch uses an outer most `namespace lld { namespace coff {` so we will not need to think much about the active namespace.
> > > > Does the argument mean this patch should be reverted?
> > > 
> > > Maybe. I'm saying I would prefer to go the opposite direction from this patch, and standardize on the `lld::coff::foo` names. But, this is just my opinion, not a standard, and I want to see if people agree first.
> > > 
> > > > If we have interleaved classes and free functions, we may have:
> > > > ...
> > > 
> > > The code pattern we had before this change looked like:
> > > ```
> > > // Foo.h
> > > namespace lld { namespace coff {
> > > class Foo { void bar(); };
> > > void baz();
> > > } } // lld::coff
> > > 
> > > // Foo.cpp
> > > using namespace lld;
> > > using namespace lld::coff;
> > > void Foo::bar() {
> > >   ...
> > > }
> > > void lld::coff::baz() {
> > >   ...
> > > }
> > > ```
> > > We never needed to open and close the namespaces in the first place, because classes like Foo are already in scope. In general, when do we have to open namespace in a .cpp file? I don't think there are any cases that really matter.
> > > 
> > > I guess another thing that makes me lean this way is the LLVM preference for as few scopes as possible:
> > > - use early return/break/continue
> > > - prefer static to anon namespace
> > > 
> > > This style seems like it fits into that: 
> > > - have as few open namespace scopes as possible
> > To gather feedback: @aganea @inglorion @pcc  @zturner 
> + at thakis
My 2¢: Fully qualifying a definition in TUs is stylistically ugly and takes more visual space, but I would tend to agree with @rnk. Omitting the namespace/scope adds cognitive load - for a moment you would ask yourself where does that implementation belong to. The code has to be easy to read, not easy to write. Same comment would apply for `#ifdef`s surrounding large blocks of code; or in-class function definitions. You end up losing the scope while reading the code. LoCs in any codebase have a natural tendency to increase, not decrease, so the situation generally worsens over time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68772