[PATCH] D15191: [ELF] Support PHDRS command

[Denis Protivensky via llvm-commits]

denis-protivensky added inline comments.

================
Comment at: ELF/Writer.cpp:1176
@@ +1175,3 @@
+    layoutCustomPhdrs(VA, FileOff);
+  else {
+    // The first phdr entry is PT_PHDR which describes the program header
----------------
grimar wrote:
> Wouldn`t it be better to write something like:
> 
> ```
>  if (hasCustomPhdrs()) {
>     layoutCustomPhdrs(VA, FileOff);
>     return;
>  }
> 
> ```
> And just copy a piece of left logic from below to layoutCustomPhdrs() or even to separate it to another method ?
> 
> Also in currect variant bracket is missed:
> ```
>  if (hasCustomPhdrs()) {
> ```
> 
No, it's not better, because there already were cases when the common logic below was extended. This would have caused logical issue in the case you propose.

Concerning the braces - I don't remember us putting them for one-line operator blocks.

================
Comment at: ELF/Writer.cpp:1217
@@ +1216,3 @@
+                                ? (uint32_t)PT_LOAD
+                                : getAmdgpuPhdr(Sec);
+          setPhdr(PH, PTType, Flags, FileOff, VA, 0, Target->getPageSize());
----------------
grimar wrote:
> Change is not relative to this patch (fix of formatting).
It's related, because you don't see that the whole code moved by one tab left (by default, ignore space mode is on). So this is the result of clang-formatting the whole shifted piece.

================
Comment at: ELF/Writer.cpp:1469
@@ -1301,3 +1468,3 @@
        Config->OutputSections)
-    for (StringRef Name : OutSec.second)
+    for (StringRef Name : OutSec.second.InputSections) {
       InputToOutputSection[Name] = OutSec.first;
----------------
grimar wrote:
> There should be brackets I think:
> 
> ```
> for (const std::pair<StringRef, OutputSectionDescription> &OutSec :
>        Config->OutputSections) {
> ...
> ```
No, I was told to remove these earlier. We don't use braces for blocks that constitute of one operator.


http://reviews.llvm.org/D15191