This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented comparsion operators for linkerscript
ClosedPublic

Authored by grimar on Apr 22 2016, 9:51 AM.

Details

Summary

Patch implements <,>,!=,==,>=,<= operators.
Actually in freebsd scripts I think I saw only != operator,
but since it is easy to support all at once - desided to do that.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 54671.Apr 22 2016, 9:51 AM
grimar retitled this revision from to [ELF] - Implemented comparsion operators for linkerscript.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added inline comments.Apr 22 2016, 10:02 AM
ELF/LinkerScript.cpp
152 ↗(On Diff #54671)

Trivial comment, but why not L != R here? And, why not just return L < R;, return L > R; etc. in general? For me at least ? 1 : 0 is not any more clear/readable.

grimar added inline comments.Apr 22 2016, 10:06 AM
ELF/LinkerScript.cpp
152 ↗(On Diff #54671)

I think you right. I`ll change that.

grimar updated this revision to Diff 54674.Apr 22 2016, 10:10 AM
  • Addressed comment from Ed.
ruiu added inline comments.Apr 22 2016, 1:22 PM
ELF/LinkerScript.cpp
51–61 ↗(On Diff #54674)

These operator precedences are not correct. * and / should have the same precedence, and so are + and -. & has lower precedence over comparison operators.

https://www.sourceware.org/binutils/docs-2.10/ld_3.html#SEC45

ruiu added inline comments.Apr 22 2016, 1:59 PM
ELF/LinkerScript.cpp
51–61 ↗(On Diff #54674)

Also, please don't use precedence 0. We pass 0 as the minimum precedence in parseExpr. It doesn't expect that there is an operator with precedence 0.

grimar added inline comments.Apr 22 2016, 2:37 PM
ELF/LinkerScript.cpp
51–61 ↗(On Diff #54674)

Yes, I messed up these accidentally :( will fix.

grimar updated this revision to Diff 54734.Apr 22 2016, 2:57 PM
  • Addressed review comments.
ELF/LinkerScript.cpp
51–61 ↗(On Diff #54674)

I used c++ precedence for added ones, btw: http://en.cppreference.com/w/cpp/language/operator_precedence.
It is a bit different about == and !=.
And I just missed that & should have lower precedence than them all.

ruiu accepted this revision.Apr 22 2016, 3:02 PM
ruiu edited edge metadata.

LGTM with a nit.

ELF/LinkerScript.cpp
121–147 ↗(On Diff #54734)

Let's sort them in the same order as the operator precedence.

This revision is now accepted and ready to land.Apr 22 2016, 3:02 PM
This revision was automatically updated to reflect the committed changes.