[PATCH] D17529: ELF: Implement ICF.

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 25 00:06:27 PST 2016


grimar added inline comments.

================
Comment at: ELF/ICF.cpp:346
@@ +345,3 @@
+  for (;;) {
+    ++Cnt;
+    uint64_t Id = NextId;
----------------
Knowing that you like to reduce the code:

```
while (++Cnt) {
...
```

================
Comment at: ELF/ICF.cpp:364
@@ +363,3 @@
+    if (Config->Verbose)
+      llvm::outs() << "Selected " << Head->getSectionName() << "\n";
+    while (I != Bound) {
----------------
What about to separate verbose() method to error.cpp file ?
Something alike:
```
void verbose(Twine Msg) {
  if (Config->Verbose)
    llvm::outs() << Msg;
}
```
Then it can be used from another code.

================
Comment at: ELF/InputSection.h:51
@@ +50,3 @@
+
+  // This pointer points to the "real" instance of this instnace.
+  // Usually Repl == this. However, if ICF merges two sections,
----------------
s/instnace/instance

================
Comment at: ELF/InputSection.h:54
@@ +53,3 @@
+  // Repl pointer of one section points to another section. So,
+  // if you need to get a pointer to this instnace, do not use
+  // this but instead this->Repl.
----------------
ditto

================
Comment at: ELF/InputSection.h:175
@@ -161,1 +174,3 @@
+  // Used by ICF.
+  uint64_t GroupId = 0;
 };
----------------
This comment is very tight. It has no value as anyone who look at this variable can quick search and find that it is used somewhere in ICF.
I suggest to expand or to remove it.


http://reviews.llvm.org/D17529





More information about the llvm-commits mailing list