[PATCH] D38846: [ELF] - Linkerscript: fix issue with SUBALIGN.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/ScriptParser.cpp:647
+  return [=] {
+    uint64_t Alignment = std::max((uint64_t)1, E().getValue());
+    if (!isPowerOf2_64(Alignment)) {
----------------
ruiu wrote:
> The use of std::max is not obvious. I prefer
> 
>   uint64_t Alignment = E().getValue();
>   if (Alignment == 0)
>     return (uint64_t)1;
>   if (!isPowerOf2_64...
Isn't it the same as we already do here:
https://github.com/llvm-mirror/lld/blob/master/ELF/ScriptParser.cpp#L964
and here:
https://github.com/llvm-mirror/lld/blob/master/ELF/ScriptParser.cpp#L994
?

Should we fix that places ?


================
Comment at: ELF/ScriptParser.cpp:650
+      error(Loc + ": alignment must be power of 2");
+      return (uint64_t)1; // Return a dummy value.
+    }
----------------
ruiu wrote:
> Remove the comment because it is obvious.
You asked me to add such comment for different but similar place here:
https://reviews.llvm.org/D36140?id=109103#827418

Isn't it consistent ? Should we remove it from there ?


https://reviews.llvm.org/D38846