[PATCH] D51671: [ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/OutputSections.h:132
   static uint8_t First;
-  static PhdrEntry *TlsPhdr;
+  static const PhdrEntry *TlsPhdr;
   static OutputSection *DebugInfo;
----------------
Seems like an unrelated change.


================
Comment at: ELF/Writer.cpp:1662
+
+    for (const PhdrEntry *P : Phdrs)
+      if (P->p_type == PT_TLS && P->FirstSec) {
----------------
We normally don't add `const`; instead we keep variable scope as short as possible to make it obviously const. Please drop it.


================
Comment at: ELF/Writer.cpp:1662
+
+    for (const PhdrEntry *P : Phdrs)
+      if (P->p_type == PT_TLS && P->FirstSec) {
----------------
ruiu wrote:
> We normally don't add `const`; instead we keep variable scope as short as possible to make it obviously const. Please drop it.
For consistency, add {}. We do

  for (...)
    if (...)
      ...;

but don't do this:

  for (...)
    if (...) {
      ...
    }

But perhaps the best way of writing this code is

  for (...) {
    if (!condition)
      continue;
    ...;
    break;
  }


================
Comment at: ELF/Writer.cpp:1662
+
+    for (const PhdrEntry *P : Phdrs)
+      if (P->p_type == PT_TLS && P->FirstSec) {
----------------
ruiu wrote:
> ruiu wrote:
> > We normally don't add `const`; instead we keep variable scope as short as possible to make it obviously const. Please drop it.
> For consistency, add {}. We do
> 
>   for (...)
>     if (...)
>       ...;
> 
> but don't do this:
> 
>   for (...)
>     if (...) {
>       ...
>     }
> 
> But perhaps the best way of writing this code is
> 
>   for (...) {
>     if (!condition)
>       continue;
>     ...;
>     break;
>   }
It needs comments. Essentially, all nontrivial code needs comments so that the code can be read and understood by a first-time reader.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51671