[PATCH] D19237: ELF: Add +, -, *, / and & to SECTIONS linker script command.

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 19 05:10:36 PDT 2016


grimar added a comment.

It was really interesting to debug for me. 
But honestly speaking I find my version much more readable.
It does not have recursion, and it is straightforward:
it builds RPN and calculates it, 2 logically separated calls at fact.

Your code works and avoids building intermediate stuff, 
but I found 3 call of parseExpr and 3 call of parsePrimary,
all from different places and also recursion involved and makes it hard to debug.
You pass MinPrec which changes because of recursion calls, 
Dot is required parameter for each of 3 methods. 
That does not look straightforward, in overall feels much more 
complicated when debugging non trivial expressions for me.

At the same time both approach should work about the same speed 
(and both are very fast), complexity of mine is also O(N), 
the fact of building RPN in the middle is greatly improves readability.
Mine version has more code, but that can be solved by placing operators
handlers in one method, like you did.

So I would really prefer mine version here(after few tweaks). You deside.


http://reviews.llvm.org/D19237





More information about the llvm-commits mailing list