Skip to content

Implement GccAsmStatement for gcc-style asm statements#8538

Merged
dlang-bot merged 5 commits intodlang:masterfrom
ibuclaw:iasmgcc
Aug 14, 2018
Merged

Implement GccAsmStatement for gcc-style asm statements#8538
dlang-bot merged 5 commits intodlang:masterfrom
ibuclaw:iasmgcc

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Aug 5, 2018

The AST itself is only used by GDC.

Parser and semantic analysis has been merged into a single function, and put in its own module as per #7562 (comment).

Lots more documentation could be added to the new functions that implement gcc asm.

Closes #7562.

@ibuclaw ibuclaw added the Compiler:GDC Gnu D Compiler label Aug 5, 2018
@ibuclaw ibuclaw requested a review from WalterBright August 5, 2018 22:41
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8538"

@ibuclaw ibuclaw force-pushed the iasmgcc branch 2 times, most recently from 94f63ef to 835a9ab Compare August 5, 2018 22:51
@ibuclaw ibuclaw requested a review from RazvanN7 as a code owner August 5, 2018 22:51
@ibuclaw ibuclaw force-pushed the iasmgcc branch 3 times, most recently from e3f7296 to e8b9957 Compare August 7, 2018 07:44
@ibuclaw ibuclaw changed the title Implement ExtAsmStatement for gcc-style asm statements Implement GccAsmStatement for gcc-style asm statements Aug 7, 2018
@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 7, 2018

As title update suggests, renamed class to GccAsmStatement to avoid ambiguity.

Added ddoc style comments, along with grammar outline of the parser.

@ibuclaw ibuclaw force-pushed the iasmgcc branch 4 times, most recently from f67ab65 to a3815e7 Compare August 8, 2018 06:37
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this commit by commit was really straight-forward. Thank you!

goto Ldone;

// No semicolon followed after instruction template, treat as extended asm.
for (int section = 0; section < 4; ++section)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an foreach ;-)

// Analyse all input and output operands.
if (s.args)
{
for (size_t i = 0; i < s.args.dim; i++)
Copy link
Contributor

@wilzbach wilzbach Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a foreach (and the two for loops below too)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean foreach (i; 0 .. s.args.dim), then ok.

@ibuclaw ibuclaw force-pushed the iasmgcc branch 2 times, most recently from 82d8eac to db2ff45 Compare August 13, 2018 20:31
@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 13, 2018

Rebased; tweaked documented grammar so that there is no distinction between input and output operands; use foreach in a few places.

@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 13, 2018

@WalterBright - This is good to review I guess.

@jacob-carlborg
Copy link
Contributor

Can tests be added?

@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 14, 2018

@jacob-carlborg unittests mimicking gdc? I guess so...

@jacob-carlborg
Copy link
Contributor

jacob-carlborg unittests mimicking gdc? I guess so..

I had a few ideas:

  1. Use more of an integration/unit test style approach instead of the current end-to-end
  2. Tests that shows what happens if DMD is trying to use this new statement
  3. Can we have tests that are only executed by GDC? We are running GDC using SemaphoreCI, as far as I know

Unless somebody has a better idea.

@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 14, 2018

Use more of an integration/unit test style approach instead of the current end-to-end

See latest commit.

Tests that shows what happens if DMD is trying to use this new statement

DMD will always treat it like a dmd asm statement, so will error in same way as before.

Can we have tests that are only executed by GDC? We are running GDC using SemaphoreCI, as far as I know

That doesn't really help at all if we make changes to it, as for proper testing you should build GDC from source. You should probably have some faith in me that I vet my own compiler. :-)

@ibuclaw ibuclaw force-pushed the iasmgcc branch 2 times, most recently from 32e8394 to 5dc7986 Compare August 14, 2018 07:52
@jacob-carlborg
Copy link
Contributor

That doesn't really help at all if we make changes to it, as for proper testing you should build GDC from source. You should probably have some faith in me that I vet my own compiler

Yes, but you're making changes to a project without adding tests to the project. But now I see that you've added tests 👍.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the code and git commit separation!

@wilzbach
Copy link
Contributor

SemaphoreCI failure is due to it checking out the wrong branch. This should be fixed by #8560 (once it's merged into stable and merged to master).

@ibuclaw
Copy link
Member Author

ibuclaw commented Aug 14, 2018

:-O

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants