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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68772