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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Please commit.



================
Comment at: ELF/ScriptParser.cpp:647
+  return [=] {
+    uint64_t Alignment = std::max((uint64_t)1, E().getValue());
+    if (!isPowerOf2_64(Alignment)) {
----------------
grimar wrote:
> 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 ?
I'll do.


================
Comment at: ELF/ScriptParser.cpp:650
+      error(Loc + ": alignment must be power of 2");
+      return (uint64_t)1; // Return a dummy value.
+    }
----------------
grimar wrote:
> 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 ?
I'll do.


https://reviews.llvm.org/D38846