lll: do not expose push/dup/swap/jumpdest as functions#3005
Conversation
|
The reason here is that none of these can be used meaningfully as each function starts its own stack frame and that means the only way to call it is |
|
Hm, should this include jump/jumpi too? |
liblll/CodeFragment.cpp
Outdated
| bool validFunctionalInstruction(string us) | ||
| { | ||
| auto it = c_instructions.find(us); | ||
| if ( |
There was a problem hiding this comment.
I'm not sure what's more readable, but in general if you are returning bool you don't need to wrap bool expression with if.
|
|
||
| namespace | ||
| { | ||
| bool validFunctionalInstruction(string us) |
There was a problem hiding this comment.
This function assumes the us is a valid instruction name. This precondition should be at least put in a comment.
Alternatively, you can handle also invalid instructions and reduce the condition in else if (c_instructions.count(us) && validFunctionalInstruction(us)).
There was a problem hiding this comment.
Handle as in? They do not make sense to be exposed as macros (aka "functions").
There was a problem hiding this comment.
Handle as if (it == c_instructions.end()) return false;.
There was a problem hiding this comment.
Well, it can be added, though the callee does that already (via count(us)). A comment will not hurt anyway.
There was a problem hiding this comment.
I'm suggesting to actually remove count(us) on the callee side. Use only validFunctionalInstruction().
There was a problem hiding this comment.
For readability purposes I'm not sold to remove it from the caller side.
There was a problem hiding this comment.
But did simplify it according to suggestions.
| if (!c_instructions.count(us)) | ||
| return false; | ||
| auto it = c_instructions.find(us); | ||
| return !( |
There was a problem hiding this comment.
Alternatively if (it == nullptr) return false; could remove the need of count()?
There was a problem hiding this comment.
Yes. Precisely: if (it == c_instructions.end()). See http://en.cppreference.com/w/cpp/container/map/find "return value" section.
liblll/CodeFragment.cpp
Outdated
|
|
||
| namespace | ||
| { | ||
| /// Returns true iff the instruction is valid as a function. Assumes @a us is not nullptr. |
There was a problem hiding this comment.
Sorry, but Assumes @a us is not nullptr. is wrong. The string cannot be nullptr, but can be empty. Anyway, there are not assumptions about the argument, because .find() will work for any input.
2 participants
Depends on #3004.
They are still available in assembly:
(asm 0 DUP1 SWAP2)