Improve command classification

Dr-Carlos commented

2022-04-14 13:54:59 +02:00

Collaborator

Hello!

Given that there hadn't been much activity on fxos in the past week (which is fine, there are plenty of other projects to work on), I thought I would try and implement issue #3. I have:

Moved dtl to .dt
Moved is to isc and sl to is
Merged sa and ss to create ma
Moved af4 and afh to s4 and sh
Removed vct in favour of vc and .
Merged ev into e vspace=

Note that ma does not support expressions - only addresses. I couldn't figure out how to do this, but I'm sure that you can do it (or add it to the parser if it isn't possible yet).

Let me know if there are any command names or descriptions that you want me to change.

If you want to test it out, I changed the commands to use the updated names in my fxos files and uploaded them to my fork of fxdoc.

Hello! Given that there hadn't been much activity on fxos in the past week (which is fine, there are plenty of other projects to work on), I thought I would try and implement issue #3. I have: - Moved `dtl` to `.dt` - Moved `is` to `isc` and `sl` to `is` - Merged `sa` and `ss` to create `ma` - Moved `af4` and `afh` to `s4` and `sh` - Removed `vct` in favour of `vc` and `.` - Merged `ev` into `e vspace=` Note that `ma` does not support expressions - only addresses. I couldn't figure out how to do this, but I'm sure that you can do it (or add it to the parser if it isn't possible yet). Let me know if there are any command names or descriptions that you want me to change. If you want to test it out, I changed the commands to use the updated names in my fxos files and uploaded them to my fork of [fxdoc](https://gitea.planet-casio.com/Dr-Carlos/fxdoc).

Lephenixnoir commented

2022-04-14 16:55:58 +02:00

Owner

Wow, thanks! This is really nice. Getting small stuff done is always more time for important features. :)

I have a single issue with your diff, which is that your editor changed a lot of the style, indent, etc. which looks really inconsistent now. This is annoying, but not as much as constantly having to worry about it, so I set up a clang-format on the master branch to automate it.

You can force the formatting without redoing your full PR like this:

% git show master:.clang-format > .clang-format
% clang-format -i include/**/*.h lib/**/*.cpp shell/**/*.cpp
% git add -u && git add .clang-format
% git commit --amend --no-edit
% git merge master

This would leave you with a couple conflicts where you essentially just need to remove the old commands. There are 3 files that stand out due to clang-format off blocks, for these please grab the unformatted block from the master branch.

Apart from that this seems to be all clean! I will update the fxdoc repository just after merging this so we're still up-to-date. Thanks for your time, and sorry for not setting up formatting before.

Wow, thanks! This is really nice. Getting small stuff done is always more time for important features. :) I have a single issue with your diff, which is that your editor changed a lot of the style, indent, etc. which looks really inconsistent now. This is annoying, but not as much as constantly having to worry about it, so I set up a clang-format on the `master` branch to automate it. You can force the formatting without redoing your full PR like this: ``` % git show master:.clang-format > .clang-format % clang-format -i include/**/*.h lib/**/*.cpp shell/**/*.cpp % git add -u && git add .clang-format % git commit --amend --no-edit % git merge master ``` This would leave you with a couple conflicts where you essentially just need to remove the old commands. There are 3 files that stand out due to `clang-format off` blocks, for these please grab the unformatted block from the `master` branch. Apart from that this seems to be all clean! I will update the fxdoc repository just after merging this so we're still up-to-date. Thanks for your time, and sorry for not setting up formatting before.

Dr-Carlos force-pushed classification from aea85260fb to d943ba12c3

2022-04-14 23:32:55 +02:00

Compare

Dr-Carlos commented

2022-04-14 23:34:56 +02:00

Author

Collaborator

Wow, thanks! This is really nice. Getting small stuff done is always more time for important features. :)

I have a single issue with your diff, which is that your editor changed a lot of the style, indent, etc. which looks really inconsistent now. This is annoying, but not as much as constantly having to worry about it, so I set up a clang-format on the master branch to automate it.

You can force the formatting without redoing your full PR like this:
% git show master:.clang-format > .clang-format
% clang-format -i include/**/*.h lib/**/*.cpp shell/**/*.cpp
% git add -u && git add .clang-format
% git commit --amend --no-edit
% git merge master
This would leave you with a couple conflicts where you essentially just need to remove the old commands. There are 3 files that stand out due to clang-format off blocks, for these please grab the unformatted block from the master branch.

Apart from that this seems to be all clean! I will update the fxdoc repository just after merging this so we're still up-to-date. Thanks for your time, and sorry for not setting up formatting before.

I'm glad you like it. I have fixed up the formatting and merged the branches. Hopefully this will avoid any code style issues in the future!

> Wow, thanks! This is really nice. Getting small stuff done is always more time for important features. :) > > I have a single issue with your diff, which is that your editor changed a lot of the style, indent, etc. which looks really inconsistent now. This is annoying, but not as much as constantly having to worry about it, so I set up a clang-format on the `master` branch to automate it. > > You can force the formatting without redoing your full PR like this: > > ``` > % git show master:.clang-format > .clang-format > % clang-format -i include/**/*.h lib/**/*.cpp shell/**/*.cpp > % git add -u && git add .clang-format > % git commit --amend --no-edit > % git merge master > ``` > > This would leave you with a couple conflicts where you essentially just need to remove the old commands. There are 3 files that stand out due to `clang-format off` blocks, for these please grab the unformatted block from the `master` branch. > > Apart from that this seems to be all clean! I will update the fxdoc repository just after merging this so we're still up-to-date. Thanks for your time, and sorry for not setting up formatting before. I'm glad you like it. I have fixed up the formatting and merged the branches. Hopefully this will avoid any code style issues in the future!

Lephenixnoir commented

2022-04-15 12:28:33 +02:00

Owner

Perfect! This diff is pristine now, and you really implemented everything. I don't see any more commands that need changing.

I merged on the command-line, though apparently not in the exact way that Gitea expected. Oops. ^^"

I have only renamed ma into ms (metadata symbol) because I intend to have other kinds of metadata, specifically types (with an associated mt command).

Note that ma does not support expressions - only addresses. I couldn't figure out how to do this, but I'm sure that you can do it (or add it to the parser if it isn't possible yet).

You can parse expressions, but currently the Symbol struct only supports fixed addresses. Pointing to symbolic expressions would be bothersome.

If you just want to allow command-time computation for the address, you could do like e and use parser.expr(). But this would resolve syscalls, so you'd need to watch the lookahead, something like (not tested):

static FxOS::Symbol parse_ms(Session &, Parser &parser)
{
    FxOS::Symbol s;
    
    if(parser.lookahead().type == T::SYSCALL) {
        s.type = FxOS::Symbol::Syscall;
        s.value = parser.expect(T::SYSCALL).value.NUM;
    }
    else {
        s.type = FxOS::Symbol::Address;
        s.value = parser.expr();
    }
    s.name = parser.symbol();

    parser.end();
    return s;
}

This is possible because composite expressions must start with (.

Perfect! This diff is pristine now, and you really implemented everything. I don't see any more commands that need changing. I merged on the command-line, though apparently not in the exact way that Gitea expected. Oops. ^^" I have only renamed `ma` into `ms` (metadata symbol) because I intend to have other kinds of metadata, specifically types (with an associated `mt` command). > Note that `ma` does not support expressions - only addresses. I couldn't figure out how to do this, but I'm sure that you can do it (or add it to the parser if it isn't possible yet). You can parse expressions, but currently the `Symbol` struct only supports fixed addresses. Pointing to symbolic expressions would be bothersome. If you just want to allow command-time computation for the address, you could do like `e` and use `parser.expr()`. But this would resolve syscalls, so you'd need to watch the lookahead, something like (not tested): ```cpp static FxOS::Symbol parse_ms(Session &, Parser &parser) { FxOS::Symbol s; if(parser.lookahead().type == T::SYSCALL) { s.type = FxOS::Symbol::Syscall; s.value = parser.expect(T::SYSCALL).value.NUM; } else { s.type = FxOS::Symbol::Address; s.value = parser.expr(); } s.name = parser.symbol(); parser.end(); return s; } ``` This is possible because composite expressions must start with `(`.

Dr-Carlos added 1 commit 2022-04-15 22:44:39 +02:00

b7d9d82482 _ms: evaluate expressions

Dr-Carlos commented

2022-04-15 22:47:01 +02:00

Author

Collaborator

You can parse expressions, but currently the Symbol struct only supports fixed addresses. Pointing to symbolic expressions would be bothersome.

If you just want to allow command-time computation for the address, you could do like e and use parser.expr(). But this would resolve syscalls, so you'd need to watch the lookahead, something like (not tested):
static FxOS::Symbol parse_ms(Session &, Parser &parser)
{
    FxOS::Symbol s;
    
    if(parser.lookahead().type == T::SYSCALL) {
        s.type = FxOS::Symbol::Syscall;
        s.value = parser.expect(T::SYSCALL).value.NUM;
    }
    else {
        s.type = FxOS::Symbol::Address;
        s.value = parser.expr();
    }
    s.name = parser.symbol();

    parser.end();
    return s;
}
This is possible because composite expressions must start with (.

Thanks! I tried to do this originally, but didn't realise I could lookahead without popping the token. I have fixed this and pushed it.

> You can parse expressions, but currently the `Symbol` struct only supports fixed addresses. Pointing to symbolic expressions would be bothersome. > > If you just want to allow command-time computation for the address, you could do like `e` and use `parser.expr()`. But this would resolve syscalls, so you'd need to watch the lookahead, something like (not tested): > > ```cpp > static FxOS::Symbol parse_ms(Session &, Parser &parser) > { > FxOS::Symbol s; > > if(parser.lookahead().type == T::SYSCALL) { > s.type = FxOS::Symbol::Syscall; > s.value = parser.expect(T::SYSCALL).value.NUM; > } > else { > s.type = FxOS::Symbol::Address; > s.value = parser.expr(); > } > s.name = parser.symbol(); > > parser.end(); > return s; > } > ``` > This is possible because composite expressions must start with `(`. Thanks! I tried to do this originally, but didn't realise I could lookahead without popping the token. I have fixed this and pushed it.

Lephenixnoir merged commit a6b66f380d into master

2022-04-15 22:57:40 +02:00

Lephenixnoir referenced this issue from a commit

2022-04-15 22:57:41 +02:00

Merge pull request 'Improve command classification' (#4) from Dr-Carlos/fxos:classification into master

Lephenixnoir commented

2022-04-15 22:58:39 +02:00

Owner

Excellent, that new interface is going to work much better.

Lephenixnoir referenced this pull request

2022-04-16 09:52:32 +02:00

Improve command classification #3

Improve command classification #4