[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 a subscriber: thakis.
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);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68772