Page MenuHomePhabricator

[COFF] Wrap things in namespace lld { namespace coff {
ClosedPublic

Authored by MaskRay on Oct 10 2019, 2:51 AM.

Diff Detail

Event Timeline

MaskRay created this revision.Oct 10 2019, 2:51 AM

LGTM, but I'll defer to @ruiu to formally approve.

ruiu accepted this revision.Oct 10 2019, 3:39 AM

LGTM

This revision is now accepted and ready to land.Oct 10 2019, 3:39 AM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Oct 10 2019, 1:19 PM
lld/COFF/DebugTypes.cpp
213–214

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.

MaskRay marked an inline comment as done.Oct 10 2019, 8:42 PM
MaskRay added inline comments.
lld/COFF/DebugTypes.cpp
213–214

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.

rnk added inline comments.Oct 11 2019, 1:49 PM
lld/COFF/DebugTypes.cpp
213–214

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
MaskRay marked an inline comment as done.Oct 16 2019, 8:57 PM
MaskRay added subscribers: aganea, inglorion, zturner, pcc.
MaskRay added inline comments.
lld/COFF/DebugTypes.cpp
213–214

To gather feedback: @aganea @inglorion @pcc @zturner

MaskRay marked an inline comment as done.Oct 16 2019, 8:59 PM
MaskRay added a subscriber: thakis.
MaskRay added inline comments.
lld/COFF/DebugTypes.cpp
213–214

Similar to D67323

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

lld/COFF/DebugTypes.cpp
213–214

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 #ifdefs 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.

rnk added a comment.Oct 17 2019, 10:12 AM

I guess the productive thing to do would be for me to put together a patch to CodingStandards and then send it to llvm-dev to get some feedback, and then follow up myself here.

In D68772#1713143, @rnk wrote:

I guess the productive thing to do would be for me to put together a patch to CodingStandards and then send it to llvm-dev to get some feedback, and then follow up myself here.

Thanks! That will be very helpful. (I wanted to do the same thing but does not have enough time to do this because I am currently busy with personal (or work related) issues..) I will clean up the ELF and wasm patches (mea culpa) once the style reaches a consensus.