[PATCH] D17529: ELF: Implement ICF.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/ICF.cpp:120
@@ +119,3 @@
+
+// Returns the number of relocations that S has.
+template <class ELFT> uint64_t ICF<ELFT>::numRels(InputSection<ELFT> *S) {
----------------
rafael wrote:
> The comment is out of date.
Done.

================
Comment at: ELF/ICF.cpp:123
@@ +122,3 @@
+  uint64_t Ret = 0;
+  for (size_t I = 0, E = S->RelocSections.size(); I != E; ++I)
+    Ret += S->RelocSections[I]->sh_size;
----------------
rafael wrote:
> Range loop?
Done.

================
Comment at: ELF/ICF.cpp:346
@@ +345,3 @@
+  for (;;) {
+    ++Cnt;
+    uint64_t Id = NextId;
----------------
grimar wrote:
> Knowing that you like to reduce the code:
> 
> ```
> while (++Cnt) {
> ...
> ```
I love short and readable code but I don't think it's readable. `Cnt` is not a loop terminating condition and it just happened to be always true (so as a result it happens to work.) Your code is semantically equivalent to

  for (;;) {
    if (++Cnt)
      break;

which is rather confusing.

================
Comment at: ELF/ICF.cpp:364
@@ +363,3 @@
+    if (Config->Verbose)
+      llvm::outs() << "Selected " << Head->getSectionName() << "\n";
+    while (I != Bound) {
----------------
grimar wrote:
> 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.
That may be a good idea. I'm not going to do that in this patch, but it worths trying.

================
Comment at: ELF/InputSection.h:175
@@ -161,1 +174,3 @@
+  // Used by ICF.
+  uint64_t GroupId = 0;
 };
----------------
grimar wrote:
> 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.
I think the comments are valuable for first-time readers.


http://reviews.llvm.org/D17529