[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 subscribers: aganea, inglorion, zturner, pcc.
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:
> 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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68772