Remove dr and integrate into d #11

Dr-Carlos · 2022-12-05T06:30:18+01:00

Dr-Carlos commented

2022-12-05 06:30:18 +01:00

Hello!

Looking back over issue #3, I remember a plan that dr would be removed and d would now be able to accept an address or a range.

This PR plans to implement this plan.

Currently I have removed dr and added token options to the parser's atom() and expr() functions. These are optional and mean that atom() uses that token instead of expecting one, allowing d to try and get a range from the parser, then fall back on the first token (saved using lookahead) if it fails.

Whilst this works, it unfortunately doesn't work with arithemtic. For example, d $, d $:0x100, d %0 ..%2, and d %100 all work. However, d ($+100) doesn't, raising the error expected '$', '(', '-', symbol, number, or syscall number; instead found end of file.

Hence, this PR is WIP as I am unsure how to fix this issue. If you have any suggestions, please let me know.

Hello! Looking back over issue #3, I remember a plan that `dr` would be removed and `d` would now be able to accept an address or a range. This PR plans to implement this plan. Currently I have removed dr and added token options to the parser's atom() and expr() functions. These are optional and mean that atom() uses that token instead of expecting one, allowing `d` to try and get a range from the parser, then fall back on the first token (saved using lookahead) if it fails. Whilst this works, it unfortunately doesn't work with arithemtic. For example, `d $`, `d $:0x100`, `d %0 ..%2`, and `d %100` all work. However, `d ($+100)` doesn't, raising the error `expected '$', '(', '-', symbol, number, or syscall number; instead found end of file`. Hence, this PR is WIP as I am unsure how to fix this issue. If you have any suggestions, please let me know.

Lephenixnoir commented

2022-12-08 22:57:34 +01:00

Thanks. I must admit I am a bit confused by your approach, specifically the token argument to atom() and expr().

I have pushed on the expr-and-ranges branch what I had in mind, which is just an extra parser function which reads an expression, and then checks whether it's followed by .. or : to build a range.

Let me know if that version covers what you have tested so far. If so, we can surely merge it with the nice logic you have written on d to get rid of dr.

Thanks. I must admit I am a bit confused by your approach, specifically the `token` argument to `atom()` and `expr()`. I have pushed on the `expr-and-ranges` branch what I had in mind, which is just [an extra parser function](https://gitea.planet-casio.com/Lephenixnoir/fxos/commit/fb639962a5b5e14157e558d737e97185c6b0bad7) which reads an expression, and then checks whether it's followed by `..` or `:` to build a range. Let me know if that version covers what you have tested so far. If so, we can surely merge it with the nice logic you have written on `d` to get rid of `dr`.

Dr-Carlos force-pushed remove-dr from 3a003de190 to c66ae1d5c6

2022-12-17 11:08:22 +01:00

Compare

Dr-Carlos changed title from ~~WIP: Remove `dr` and integrate into `d`~~ to Remove `dr` and integrate into `d`

2022-12-17 11:09:33 +01:00

Dr-Carlos commented

2022-12-17 11:11:30 +01:00

Thanks for writing the expr_or_range function, it's a much better solution than what I was doing. I have changed the implementation to use this function, and it works with everything I have tested.

Ignore the weird logs about adding commits, from the 'Commits' tab you can see that it's just the three commits - my original one, yours, and the new one.

If you're fine with this, please merge it.

Thanks for writing the `expr_or_range` function, it's a much better solution than what I was doing. I have changed the implementation to use this function, and it works with everything I have tested. Ignore the weird logs about adding commits, from the 'Commits' tab you can see that it's just the three commits - my original one, yours, and the new one. If you're fine with this, please merge it.

Lephenixnoir requested changes 2022-12-17 11:21:13 +01:00

Lephenixnoir left a comment

Thanks, there's just one detail I forgot to mention about the options. I can make the change later if you prefer.

shell/d.cpp

					
				@ -82,2 +74,2 @@

				    return address;

				}

				    std::optional<uint32_t> address;

				    std::optional<Range> range;

Lephenixnoir commented

2022-12-17 11:20:17 +01:00

Can we keep the variant at this stage and just check holds_alternative in the _d function instead? This sort of suggests that we can specify both or neither, which wouldn't make much sense.

Can we keep the variant at this stage and just check `holds_alternative` in the `_d` function instead? This sort of suggests that we can specify both or neither, which wouldn't make much sense.

Dr-Carlos commented

2022-12-17 21:53:13 +01:00

Yep, makes sense. Implemented in latest commit.

Dr-Carlos marked this conversation as resolved

Dr-Carlos added 1 commit 2022-12-17 21:52:58 +01:00

111620a649 _d: determine address or range in _d

Dr-Carlos commented

2022-12-17 21:53:57 +01:00

Yeah, that makes sense. Latest commit implements this.

Lephenixnoir merged commit bbd25b625c into master

2022-12-17 21:54:48 +01:00

Lephenixnoir referenced this issue from a commit