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

[MaskRay]

Summary

Similar to D67323, but for COFF. Many lld/COFF/ files already use
namespace lld { namespace coff {. Only a few need changing.

[mstorsjo]

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

[ruiu]

LGTM

[rnk]

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

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]

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

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]

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

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]

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

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

[MaskRay]

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

+@thakis

[aganea]

Similar to D67323

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

lld/COFF/DebugTypes.cpp

213–214 ↗

(On Diff #224313)

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]

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.

[MaskRay]

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.

COFF/DebugTypes.cpp

 #include "lld/Common/ErrorHandler.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
 #include "llvm/DebugInfo/PDB/GenericError.h"
 #include "llvm/DebugInfo/PDB/Native/InfoStream.h"
 #include "llvm/DebugInfo/PDB/Native/NativeSession.h"
 #include "llvm/DebugInfo/PDB/Native/PDBFile.h"
 #include "llvm/Support/Path.h"
 
-using namespace lld;
-using namespace lld::coff;
 using namespace llvm;
 using namespace llvm::codeview;
 
+namespace lld {
+namespace coff {
+
 namespace {
 // The TypeServerSource class represents a PDB type server, a file referenced by
 // OBJ files compiled with MSVC /Zi. A single PDB can be shared by several OBJ
 // files, therefore there must be only once instance per OBJ lot. The file path
 // is discovered from the dependent OBJ's debug type stream. The
 // TypeServerSource object is then queued and loaded by the COFF Driver. The
 // debug type stream for such PDB files will be merged first in the final PDB,
 // before any dependent OBJ.

 } // namespace
 
 static std::vector<std::unique_ptr<TpiSource>> GC;
 
 TpiSource::TpiSource(TpiKind k, const ObjFile *f) : kind(k), file(f) {
   GC.push_back(std::unique_ptr<TpiSource>(this));
 }
 
-TpiSource *lld::coff::makeTpiSource(const ObjFile *f) {
+TpiSource *makeTpiSource(const ObjFile *f) {
   return new TpiSource(TpiSource::Regular, f);
 }
 
-TpiSource *lld::coff::makeUseTypeServerSource(const ObjFile *f,
+TpiSource *makeUseTypeServerSource(const ObjFile *f,
                                               const TypeServer2Record *ts) {
   TypeServerSource::enqueue(f, *ts);
   return new UseTypeServerSource(f, ts);
 }
 
-TpiSource *lld::coff::makePrecompSource(const ObjFile *f) {
+TpiSource *makePrecompSource(const ObjFile *f) {
   return new PrecompSource(f);
 }
 
-TpiSource *lld::coff::makeUsePrecompSource(const ObjFile *f,
+TpiSource *makeUsePrecompSource(const ObjFile *f,
                                            const PrecompRecord *precomp) {
   return new UsePrecompSource(f, precomp);
 }
 
-namespace lld {
-namespace coff {
 template <>
 const PrecompRecord &retrieveDependencyInfo(const TpiSource *source) {
   assert(source->kind == TpiSource::UsingPCH);
   return ((const UsePrecompSource *)source)->precompDependency;
 }
 
 template <>
 const TypeServer2Record &retrieveDependencyInfo(const TpiSource *source) {
   assert(source->kind == TpiSource::UsingPDB);
   return ((const UseTypeServerSource *)source)->typeServerDependency;
 }
-} // namespace coff
-} // namespace lld
 
 std::map<std::string, std::pair<std::string, TypeServerSource *>>
     TypeServerSource::instances;
 
 // Make a PDB path assuming the PDB is in the same folder as the OBJ
 static std::string getPdbBaseName(const ObjFile *file, StringRef tSPath) {
   StringRef localPath =
       !file->parentName.empty() ? file->parentName : file->getName();
return createFileError(
         ts.Name,
         make_error<pdb::PDBError>(pdb::pdb_error_code::signature_out_of_date));
 
   return pdb.second;
 }
 
 // FIXME: Temporary interface until PDBLinker::maybeMergeTypeServerPDB() is
 // moved here.
-Expected<llvm::pdb::NativeSession *>
-lld::coff::findTypeServerSource(const ObjFile *f) {
+Expected<llvm::pdb::NativeSession *> findTypeServerSource(const ObjFile *f) {
   Expected<TypeServerSource *> ts = TypeServerSource::findFromFile(f);
   if (!ts)
     return ts.takeError();
   return ts.get()->session.get();
 }
 
 // Queue a PDB type server for loading in the COFF Driver
 void TypeServerSource::enqueue(const ObjFile *dependentFile,
void TypeServerSource::enqueue(const ObjFile *dependentFile,
   driver->enqueuePath(*p, false, false);
 }
 
 // Create an instance of TypeServerSource or an error string if the PDB couldn't
 // be loaded. The error message will be displayed later, when the referring OBJ
 // will be merged in. NOTE - a PDB load failure is not a link error: some
 // debug info will simply be missing from the final PDB - that is the default
 // accepted behavior.
-void lld::coff::loadTypeServerSource(llvm::MemoryBufferRef m) {
+void loadTypeServerSource(llvm::MemoryBufferRef m) {
   std::string path = normalizePdbPath(m.getBufferIdentifier());
 
   Expected<TypeServerSource *> ts = TypeServerSource::getInstance(m);
   if (!ts)
     TypeServerSource::instances[path] = {toString(ts.takeError()), nullptr};
   else
     TypeServerSource::instances[path] = {{}, *ts};
 }
Expected<TypeServerSource *> TypeServerSource::getInstance(MemoryBufferRef m) {
 
   pdb::PDBFile &pdbFile = session->getPDBFile();
   Expected<pdb::InfoStream &> info = pdbFile.getPDBInfoStream();
   // All PDB Files should have an Info stream.
   if (!info)
     return info.takeError();
   return new TypeServerSource(m, session.release());
 }
+
+} // namespace coff
+} // namespace lld

COFF/InputFiles.cpp

 using namespace llvm::codeview;
 using namespace llvm::object;
 using namespace llvm::support::endian;
 
 using llvm::Triple;
 using llvm::support::ulittle32_t;
 
 namespace lld {
+
+// Returns the last element of a path, which is supposed to be a filename.
+static StringRef getBasename(StringRef path) {
+  return sys::path::filename(path, sys::path::Style::windows);
+}
+
+// Returns a string in the format of "foo.obj" or "foo.obj(bar.lib)".
+std::string toString(const coff::InputFile *file) {
+  if (!file)
+    return "<internal>";
+  if (file->parentName.empty() || file->kind() == coff::InputFile::ImportKind)
+    return file->getName();
+
+  return (getBasename(file->parentName) + "(" + getBasename(file->getName()) +
+          ")")
+      .str();
+}
+
 namespace coff {
 
 std::vector<ObjFile *> ObjFile::instances;
 std::vector<ImportFile *> ImportFile::instances;
 std::vector<BitcodeFile *> BitcodeFile::instances;
 
 /// Checks that Source is compatible with being a weak alias to Target.
 /// If Source is Undefined and has no weak alias set, makes it a weak

 std::string replaceThinLTOSuffix(StringRef path) {
   StringRef suffix = config->thinLTOObjectSuffixReplace.first;
   StringRef repl = config->thinLTOObjectSuffixReplace.second;
 
   if (path.consume_back(suffix))
     return (path + repl).str();
   return path;
 }
+
 } // namespace coff
 } // namespace lld
-
-// Returns the last element of a path, which is supposed to be a filename.
-static StringRef getBasename(StringRef path) {
-  return sys::path::filename(path, sys::path::Style::windows);
-}
-
-// Returns a string in the format of "foo.obj" or "foo.obj(bar.lib)".
-std::string lld::toString(const coff::InputFile *file) {
-  if (!file)
-    return "<internal>";
-  if (file->parentName.empty() || file->kind() == coff::InputFile::ImportKind)
-    return file->getName();
-
-  return (getBasename(file->parentName) + "(" + getBasename(file->getName()) +
-          ")")
-      .str();
-}

COFF/LTO.cpp

 #include <memory>
 #include <string>
 #include <system_error>
 #include <vector>
 
 using namespace llvm;
 using namespace llvm::object;
 
-using namespace lld;
-using namespace lld::coff;
+namespace lld {
+namespace coff {
 
 // Creates an empty file to and returns a raw_fd_ostream to write to it.
 static std::unique_ptr<raw_fd_ostream> openFile(StringRef file) {
   std::error_code ec;
   auto ret =
       std::make_unique<raw_fd_ostream>(file, ec, sys::fs::OpenFlags::OF_None);
   if (ec) {
     error("cannot open " + file + ": " + ec.message());
std::vector<StringRef> BitcodeCompiler::compile() {
   }
 
   for (std::unique_ptr<MemoryBuffer> &file : files)
     if (file)
       ret.push_back(file->getBuffer());
 
   return ret;
 }
+
+} // namespace coff
+} // namespace lld

COFF/MapFile.cpp

 #include "Writer.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Threads.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
-using namespace lld;
-using namespace lld::coff;
+namespace lld {
+namespace coff {
 
 using SymbolMapTy =
     DenseMap<const SectionChunk *, SmallVector<DefinedRegular *, 4>>;
 
 static constexpr char indent8[] = "        ";          // 8 spaces
 static constexpr char indent16[] = "                "; // 16 spaces
 
 // Print out the first three columns of a line.
getSymbolStrings(ArrayRef<DefinedRegular *> syms) {
   });
 
   DenseMap<DefinedRegular *, std::string> ret;
   for (size_t i = 0, e = syms.size(); i < e; ++i)
     ret[syms[i]] = std::move(str[i]);
   return ret;
 }
 
-void coff::writeMapFile(ArrayRef<OutputSection *> outputSections) {
+void writeMapFile(ArrayRef<OutputSection *> outputSections) {
   if (config->mapFile.empty())
     return;
 
   std::error_code ec;
   raw_fd_ostream os(config->mapFile, ec, sys::fs::OF_None);
   if (ec)
     fatal("cannot open " + config->mapFile + ": " + ec.message());
 
for (Chunk *c : sec->chunks) {
       writeHeader(os, sc->getRVA(), sc->getSize(), sc->getAlignment());
       os << indent8 << sc->file->getName() << ":(" << sc->getSectionName()
          << ")\n";
       for (DefinedRegular *sym : sectionSyms[sc])
         os << symStr[sym] << '\n';
     }
   }
 }
+
+} // namespace coff
+} // namespace lld

COFF/MinGW.cpp

 //===- MinGW.cpp ----------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MinGW.h"
 #include "SymbolTable.h"
 #include "lld/Common/ErrorHandler.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
-using namespace lld;
-using namespace lld::coff;
 using namespace llvm;
 using namespace llvm::COFF;
 
+namespace lld {
+namespace coff {
+
 AutoExporter::AutoExporter() {
   excludeLibs = {
       "libgcc",
       "libgcc_s",
       "libstdc++",
       "libmingw32",
       "libmingwex",
       "libg2c",
bool AutoExporter::shouldExport(Defined *sym) const {
   libName = libName.substr(0, libName.rfind('.'));
   if (!libName.empty())
     return !excludeLibs.count(libName);
 
   StringRef fileName = sys::path::filename(sym->getFile()->getName());
   return !excludeObjects.count(fileName);
 }
 
-void coff::writeDefFile(StringRef name) {
+void writeDefFile(StringRef name) {
   std::error_code ec;
   raw_fd_ostream os(name, ec, sys::fs::OF_None);
   if (ec)
     fatal("cannot open " + name + ": " + ec.message());
 
   os << "EXPORTS\n";
   for (Export &e : config->exports) {
     os << "    " << e.exportName << " "
        << "@" << e.ordinal;
     if (auto *def = dyn_cast_or_null<Defined>(e.sym)) {
       if (def && def->getChunk() &&
           !(def->getChunk()->getOutputCharacteristics() & IMAGE_SCN_MEM_EXECUTE))
         os << " DATA";
     }
     os << "\n";
   }
 }
+
+} // namespace coff
+} // namespace lld

COFF/PDB.cpp

 #include "llvm/Support/CRC.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include <memory>
 
-using namespace lld;
-using namespace lld::coff;
 using namespace llvm;
 using namespace llvm::codeview;
 
 using llvm::object::coff_section;
 
+namespace lld {
+namespace coff {
+
 static ExitOnError exitOnErr;
 
 static Timer totalPdbLinkTimer("PDB Emission (Cumulative)", Timer::root());
 
 static Timer addObjectsTimer("Add Objects", totalPdbLinkTimer);
 static Timer typeMergingTimer("Type Merging", addObjectsTimer);
 static Timer symbolMergingTimer("Symbol Merging", addObjectsTimer);
 static Timer globalsLayoutTimer("Globals Stream Layout", totalPdbLinkTimer);
for (ImportFile *file : ImportFile::instances) {
 
     pdb::SectionContrib sc =
         createSectionContrib(thunk->getChunk(), mod->getModuleIndex());
     mod->setFirstSectionContrib(sc);
   }
 }
 
 // Creates a PDB file.
-void coff::createPDB(SymbolTable *symtab,
+void createPDB(SymbolTable *symtab,
                      ArrayRef<OutputSection *> outputSections,
                      ArrayRef<uint8_t> sectionTable,
                      llvm::codeview::DebugInfo *buildId) {
   ScopedTimer t1(totalPdbLinkTimer);
   PDBLinker pdb(symtab);
 
   pdb.initialize(buildId);
   pdb.addObjectsToPDB();
static bool findLineTable(const SectionChunk *c, uint32_t addr,
   }
 
   return false;
 }
 
 // Use CodeView line tables to resolve a file and line number for the given
 // offset into the given chunk and return them, or {"", 0} if a line table was
 // not found.
-std::pair<StringRef, uint32_t> coff::getFileLineCodeView(const SectionChunk *c,
+std::pair<StringRef, uint32_t> getFileLineCodeView(const SectionChunk *c,
                                                          uint32_t addr) {
   ExitOnError exitOnErr;
 
   DebugStringTableSubsectionRef cVStrTab;
   DebugChecksumsSubsectionRef checksums;
   DebugLinesSubsectionRef lines;
   uint32_t offsetInLinetable;
 
for (const LineNumberEntry &ln : entry.LineNumbers) {
       lineNumber = li.getStartLine();
     }
   }
   if (!nameIndex)
     return {"", 0};
   StringRef filename = exitOnErr(getFileName(cVStrTab, checksums, *nameIndex));
   return {filename, *lineNumber};
 }
+
+} // namespace coff
+} // namespace lld

COFF/Writer.cpp

 #include <memory>
 #include <utility>
 
 using namespace llvm;
 using namespace llvm::COFF;
 using namespace llvm::object;
 using namespace llvm::support;
 using namespace llvm::support::endian;
-using namespace lld;
-using namespace lld::coff;
+
+namespace lld {
+namespace coff {
 
 /* To re-generate DOSProgram:
 $ cat > /tmp/DOSProgram.asm
 org 0
         ; Copy cs to ds.
         push cs
         pop ds
         ; Point ds:dx at the $-terminated string.
private:
   // are entirely linker-generated we can keep track of their locations using
   // the chunks that the linker creates. All .pdata chunks come from input
   // files, so we need to keep track of them separately.
   Chunk *firstPdata = nullptr;
   Chunk *lastPdata;
 };
 } // anonymous namespace
 
-namespace lld {
-namespace coff {
-
 static Timer codeLayoutTimer("Code Layout", Timer::root());
 static Timer diskCommitTimer("Commit Output File", Timer::root());
 
 void writeResult() { Writer().run(); }
 
 void OutputSection::addChunk(Chunk *c) {
   chunks.push_back(c);
 }
strncpy(hdr->Name, name.data(),
             std::min(name.size(), (size_t)COFF::NameSize));
   }
 }
 
 void OutputSection::addContributingPartialSection(PartialSection *sec) {
   contribSections.push_back(sec);
 }
 
-} // namespace coff
-} // namespace lld
-
 // Check whether the target address S is in range from a relocation
 // of type relType at address P.
 static bool isInRange(uint16_t relType, uint64_t s, uint64_t p, int margin) {
   if (config->machine == ARMNT) {
     int64_t diff = AbsoluteDifference(s, p + 4) + margin;
     switch (relType) {
     case IMAGE_REL_ARM_BRANCH20T:
       return isInt<21>(diff);

 }
 
 PartialSection *Writer::findPartialSection(StringRef name, uint32_t outChars) {
   auto it = partialSections.find({name, outChars});
   if (it != partialSections.end())
     return it->second;
   return nullptr;
 }
+
+} // namespace coff
+} // namespace lld