New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debugger: parse octal expressions (prefixed by '0') #980

Merged
merged 1 commit into from Jan 5, 2017

Conversation

Projects
None yet
6 participants
@shattered
Contributor

shattered commented Jun 20, 2016

No description provided.

@cuavas

This comment has been minimized.

Show comment
Hide comment
@cuavas

cuavas Jun 21, 2016

Member

This is going to cause a big change in behaviour as currently you can have leading zeroes on a number using default base. Could you use 0o instead?

Member

cuavas commented Jun 21, 2016

This is going to cause a big change in behaviour as currently you can have leading zeroes on a number using default base. Could you use 0o instead?

@Lord-Nightmare

This comment has been minimized.

Show comment
Hide comment
@Lord-Nightmare

Lord-Nightmare Jun 21, 2016

Contributor

Personally, I'd go for 0o as the octal prefix; most new languages are switching to that for octal, and even C and C++ standards committees are proposing deprecating the old 0 prefix in favor of 0o as it makes code more readable at a glance.

Contributor

Lord-Nightmare commented Jun 21, 2016

Personally, I'd go for 0o as the octal prefix; most new languages are switching to that for octal, and even C and C++ standards committees are proposing deprecating the old 0 prefix in favor of 0o as it makes code more readable at a glance.

@ajrhacker

This comment has been minimized.

Show comment
Hide comment
@ajrhacker

ajrhacker Jun 21, 2016

Contributor

I thought the standard Intel letter suffixes (h for hexadecimals, o or q for octals) might be a more appropriate syntax to implement in the MAME debugger. I don't know, though; perhaps it's more important to maintain bug-compatibility with C++ and the many other descendants of the typeless language Ken Thompson and Dennis Ritchie implemented on the 18-bit PDP-7.

Contributor

ajrhacker commented Jun 21, 2016

I thought the standard Intel letter suffixes (h for hexadecimals, o or q for octals) might be a more appropriate syntax to implement in the MAME debugger. I don't know, though; perhaps it's more important to maintain bug-compatibility with C++ and the many other descendants of the typeless language Ken Thompson and Dennis Ritchie implemented on the 18-bit PDP-7.

@rb6502

This comment has been minimized.

Show comment
Hide comment
@rb6502

rb6502 Jun 21, 2016

Contributor

The goal here is to be compatible with old DEC documentation, as I understand it, so the old-style leading zero is definitely desirable. Maybe 0o as the default, and have a "DEC mode" command that switches it to accept just 0?

Contributor

rb6502 commented Jun 21, 2016

The goal here is to be compatible with old DEC documentation, as I understand it, so the old-style leading zero is definitely desirable. Maybe 0o as the default, and have a "DEC mode" command that switches it to accept just 0?

@ajrhacker

This comment has been minimized.

Show comment
Hide comment
@ajrhacker

ajrhacker Jun 21, 2016

Contributor

Another idea I had was that commands that take a CPU space as a parameter could set the default number base to 8 if is_octal() holds true.

Contributor

ajrhacker commented Jun 21, 2016

Another idea I had was that commands that take a CPU space as a parameter could set the default number base to 8 if is_octal() holds true.

@shattered

This comment has been minimized.

Show comment
Hide comment
@shattered

shattered Jun 21, 2016

Contributor

RB: yes, that's the idea -- DEC PDP-11 world is octal. They don't use a prefix, though.

Making other parts of MAME honor is_octal() is bigger task, I thought I'd start with something simple :)

Contributor

shattered commented Jun 21, 2016

RB: yes, that's the idea -- DEC PDP-11 world is octal. They don't use a prefix, though.

Making other parts of MAME honor is_octal() is bigger task, I thought I'd start with something simple :)

@pullmoll

This comment has been minimized.

Show comment
Hide comment
@pullmoll

pullmoll Aug 1, 2016

Member

I'd suggest to accept prefix 0 for octal if address_space::is_octal() for the memory region in question (code, data, io) returns true, as @ajrhacker suggested. 0o prefix could be used to work in any case.

It's quite irritating if addresses printed before e.g. the dasm output are octal, and even without leading zeroes, and you have to enter decimal or 0x prefixed breakpoint adresses.

Member

pullmoll commented Aug 1, 2016

I'd suggest to accept prefix 0 for octal if address_space::is_octal() for the memory region in question (code, data, io) returns true, as @ajrhacker suggested. 0o prefix could be used to work in any case.

It's quite irritating if addresses printed before e.g. the dasm output are octal, and even without leading zeroes, and you have to enter decimal or 0x prefixed breakpoint adresses.

@rb6502

This comment has been minimized.

Show comment
Hide comment
@rb6502

rb6502 Aug 7, 2016

Contributor

I like @pullmoll 's suggestion. @shattered , if you can make it work that way I'll be glad to apply it.

Contributor

rb6502 commented Aug 7, 2016

I like @pullmoll 's suggestion. @shattered , if you can make it work that way I'll be glad to apply it.

shattered added a commit to shattered/mame that referenced this pull request Oct 3, 2016

@rb6502 rb6502 merged commit 2c7ab7f into mamedev:master Jan 5, 2017

2 checks passed

continuous-integration/tea the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cuavas

This comment has been minimized.

Show comment
Hide comment
@cuavas

cuavas Jan 6, 2017

Member

So who going to actually apply @pullmoll's suggestion before the release goes out?

Member

cuavas commented Jan 6, 2017

So who going to actually apply @pullmoll's suggestion before the release goes out?

@shattered shattered deleted the shattered:_ad3979c branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment