[PATCH] D38137: [ELF] Simpler scheme for handling common symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.



================
Comment at: ELF/Driver.cpp:1088
+  if (Config->DefineCommon)
+    lld::elf::commonToBss<ELFT>();
+
----------------
Remove `lld::elf::`. In general, please look around and make your new code looks like the same in style as other code.


================
Comment at: ELF/MarkLive.cpp:67-68
 
-  if (auto *Sym = dyn_cast<DefinedCommon>(&B)) {
-    Sym->Live = true;
-    return;
-  }
+  if (auto *Sym = dyn_cast<DefinedCommon>(&B))
+    llvm_unreachable("unreachable: common are converted to bss");
 
----------------
Just remove the code.


================
Comment at: ELF/MarkLive.cpp:229-230
     }
     if (auto *S = dyn_cast_or_null<DefinedCommon>(Sym))
-      S->Live = true;
+      llvm_unreachable("unreachable: common are converted to bss");
   };
----------------
Remove


================
Comment at: ELF/SyntheticSections.cpp:64
 
-    Sym->Section = make<BssSection>("COMMON");
-    size_t Pos = Sym->Section->reserveSpace(Sym->Size, Sym->Alignment);
+    // create a fake section for the common data
+    auto * Section = make<BssSection>("COMMON");
----------------
create -> Create

It's not a fake section. We call it a synthetic section.

Please add a full stop at end.


================
Comment at: ELF/SyntheticSections.cpp:65
+    // create a fake section for the common data
+    auto * Section = make<BssSection>("COMMON");
+    InputSections.push_back(Section);
----------------
`auto * ` -> `auto *`


================
Comment at: ELF/SyntheticSections.cpp:69-70
+    size_t Pos = Section->reserveSpace(Sym->Size, Sym->Alignment);
     assert(Pos == 0);
     (void)Pos;
+    auto * File = Sym->getFile();
----------------
Remove `Pos` and `assert`.


================
Comment at: ELF/SyntheticSections.cpp:71
     (void)Pos;
-    Sym->Section->File = Sym->getFile();
-    Ret.push_back(Sym->Section);
+    auto * File = Sym->getFile();
+
----------------
We do not use `auto` unless its actual type is obvious from RHS.


================
Comment at: ELF/SyntheticSections.cpp:73
+
+    // change the symbol to be a regular definition
+    Section->File = File;
----------------
change -> Change

Add a full stop.


https://reviews.llvm.org/D38137