[PATCH] D45467: COFF: Friendlier undefined symbol errors.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Chunks.h:195-197
+  ArrayRef<coff_relocation> relocs() const {
+    return {Relocs.begin(), Relocs.end()};
+  }
----------------
If we can always do this, should we store Relocs as an ArrayRef instead of iterator_range in the first place?


================
Comment at: lld/COFF/PDB.cpp:1161
+
+std::pair<StringRef, uint32_t> coff::getFileLine(SectionChunk *C,
+                                                 uint32_t Addr) {
----------------
Please add a function comment.


================
Comment at: lld/COFF/PDB.cpp:1170-1186
+  uint32_t SecrelReloc;
+  switch (Config->Machine) {
+  case AMD64:
+    SecrelReloc = COFF::IMAGE_REL_AMD64_SECREL;
+    break;
+  case I386:
+    SecrelReloc = COFF::IMAGE_REL_I386_SECREL;
----------------
Can you factor this code out as a separate function?


================
Comment at: lld/COFF/PDB.cpp:1188
+
+  for (SectionChunk *DbgC : C->File->getDebugChunks()) {
+    if (DbgC->getSectionName() != ".debug$S")
----------------
Maybe you can factor out this loop as a new function that returns a line table for a given address?


================
Comment at: lld/COFF/SymbolTable.cpp:68
 
+static StringRef getSymbolName(SectionChunk *SC, uint32_t Addr) {
+  StringRef CandidateName;
----------------
Add a function comment.


================
Comment at: lld/COFF/SymbolTable.cpp:77-79
+
+    CandidateValue = D->getValue();
+    CandidateName = D->getName();
----------------
Instead of storing two separate values, you might be able to just save the symbol.


================
Comment at: lld/COFF/SymbolTable.cpp:108
+  if (Locations.empty()) {
+    llvm::errs() << ">>> referenced by " << toString(File) << "\n\n";
+    return;
----------------
Use error().


https://reviews.llvm.org/D45467