Skip to content
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

adds basic legv8 snippets #8

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JoaoLira-br
Copy link

Trying to add PR to issue patrickdemers6/legv8-vscode#2

Copy link
Owner

@patrickdemers6 patrickdemers6 left a comment

Choose a reason for hiding this comment

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

There are great! This is not specific to LSP; therefore, it belongs in the vscode extension repository.

https://github.com/patrickdemers6/legv8-vscode/

Ideas for other snippets that would be nice to have:

  • call a procedure
  • for loop
  • return (it could just be an alias for BR LR)

snippets/legv8.json Outdated Show resolved Hide resolved
snippets/legv8.json Outdated Show resolved Hide resolved
snippets/legv8.json Outdated Show resolved Hide resolved
snippets/legv8.json Outdated Show resolved Hide resolved
snippets/legv8.json Outdated Show resolved Hide resolved
snippets/legv8.json Outdated Show resolved Hide resolved
"body": ["EOR ${1:target reg}, $1, $1", "$2"],
"description": "equivalent of CLEAR. It clears the content of a target register"
},
"increment offset by 8 bits": {
Copy link
Owner

Choose a reason for hiding this comment

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

What about adding a command for incrementing by immediate?

Something like:

"increment by immediate": {
  "prefix": "inci",
  "body": ["ADDI ${1:reg}, ${1}, #${2:amount}"],
  "description": "..."
}

Copy link
Author

Choose a reason for hiding this comment

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

Implemented!

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not finding this in the PR.

"body": ["ADD ${1:dest reg}, XZR, $2", "$3"]
},
"increment register": {
"prefix": "increg",
Copy link
Owner

Choose a reason for hiding this comment

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

Since it is only incrementing by one, would a prefix of reg++ or inc_1 make more sense?

Copy link
Author

Choose a reason for hiding this comment

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

I removed that snippet also

snippets/legv8.json Outdated Show resolved Hide resolved
"set register to": {
"prefix": "sreg",
"body": ["ADD ${1:dest reg}, XZR, $2", "$3"]
},
Copy link
Owner

Choose a reason for hiding this comment

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

Add description for all of them. Also try to make it easy to realize what the command does.

So if the code inserted is something to the effect of ADDI reg, reg, #1, show how to think of it in basic math terms, like reg = reg + 1

Copy link
Author

Choose a reason for hiding this comment

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

Good comment! It`s implemented

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome! In addition to the math part, could you add a small text description? Also, having the description and placeholder text be the same may be useful. Such as:

  "set register": {
    "prefix": "sreg",
    "body": ["ADD ${1:Rd}, XZR, ${2:Rm}", "$3"],
    "description": "Rd = Rm. Set one register (Rd) to the value of another (Rm)."
  }

Thoughts?

@patrickdemers6
Copy link
Owner

I transferred the issue to the proper repository. If you could create a PR using these comments to the proper repo, that would be great!

@patrickdemers6
Copy link
Owner

The issue: patrickdemers6/legv8-vscode#2

@JoaoLira-br
Copy link
Author

JoaoLira-br commented Nov 23, 2022 via email

@patrickdemers6
Copy link
Owner

No rush. Happy Thanksgiving!

@JoaoLira-br
Copy link
Author

No rush. Happy Thanksgiving!
Thank you, you too!

I transferred the issue to the proper repository. If you could create a PR using these comments to the proper repo, that would be great!

I`m not sure how and where to do this. Do you know?

Copy link
Owner

@patrickdemers6 patrickdemers6 left a comment

Choose a reason for hiding this comment

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

Great work! A few more suggestions with the modifications.

"set register to": {
"prefix": "sreg",
"body": ["ADD ${1:dest reg}, XZR, $2", "$3"]
},
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome! In addition to the math part, could you add a small text description? Also, having the description and placeholder text be the same may be useful. Such as:

  "set register": {
    "prefix": "sreg",
    "body": ["ADD ${1:Rd}, XZR, ${2:Rm}", "$3"],
    "description": "Rd = Rm. Set one register (Rd) to the value of another (Rm)."
  }

Thoughts?

"description": "R[1] = R[1] - R[2]"
},

"stack manipulation with 2 registers": {
Copy link
Owner

Choose a reason for hiding this comment

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

If I were to use this, I would expect to be able to to manipulate two registers when using st2. I completely get that there are two being manipulated here since it automatically includes LR. Do you think st2 would be more intuitive if it deals with LR and one custom register or LR and two custom registers?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking the same thing, but like you said I was stuck on the technicality of it since LR is also a register. But I think that being more intuitive is more important!

],
"description": "use this to save 8 registers in the Stack, including the return address of the procedure (where the procedure was called)"
},
"incremental for loop": {
Copy link
Owner

Choose a reason for hiding this comment

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

A few suggestions for the loop.

  • ifl isn't immediately understandable so being a bit more explicit makes it easier to grasp and remember.
  • The use of a temporary register is annoying when writing legv8 code as it requires use of another register. Using flags, this can be simplified.
  • Also not using the term arr_len and using a generic upper limit makes it clearer that this can be used for any need, not just arrays.
  • I've also modified the description to include the equivalent C code.
  "increasing for loop": {
    "prefix": "for_increasing",
    "body": [
      "${1:loop name}_loop:",
      "// if counter ($2) greater than or equal to limit ($3), then exit",
      "SUBS XZR, ${2:counter}, ${3:upper_limit}",
      "B.GE ${1}_loop_done",
      "// loop body",
      "",
      "// increment counter ($2) by $4",
      "ADDI $2, $2, #${4:increment_amount}",
      "B $1_loop",
      "$1_loop_done:"
    ],
    "description": "Creates a for loop from counter to upper_limit. for (; counter < upper_limit; counter += increment_amount); Note: the counter and array size must be initialized before the loop."
  },

Copy link
Author

Choose a reason for hiding this comment

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

I agree, It`s Implemented

"return statement": {
"prefix": "rtn",
"body": ["BR LR"],
"description": ""
Copy link
Owner

Choose a reason for hiding this comment

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

Return from this procedure to the calling procedure.

Copy link
Author

Choose a reason for hiding this comment

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

For the description you mean? I was thinking of putting a bit more compact phrase like "return to the caller". What do you think?

"description": "creates an incremental for loop. Obs: the register for the loop counter is not created with the snippet, so they must be set"
},
"return statement": {
"prefix": "rtn",
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason to not say the full return?

Copy link
Author

Choose a reason for hiding this comment

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

No, it`s just that i have a tendency to abbreviate things, but I changed to "return"

@patrickdemers6
Copy link
Owner

No rush. Happy Thanksgiving!
Thank you, you too!

I transferred the issue to the proper repository. If you could create a PR using these comments to the proper repo, that would be great!

I`m not sure how and where to do this. Do you know?

Yep, in the other VS Code extension's repository, create a pull request. You will need to:

  • Fork the repository https://github.com/patrickdemers6/legv8-vscode/
  • Clone the repository to your machine.
  • Create a branch, such as 2-snippets
  • Make your changes. I would create a folder called snippets, then a file named legv8.snippets.json. Then, add the below to contributes in package.json. This tells the extension that there are snippets found in this file.
    "snippets": [
      {
        "language": "legv8",
        "path": "./snippets/legv8.snippets.json"
      }
  • Next, test your changes. In VS Code, launch the extension and make sure it all looks as you expect when in use.
  • Commit your changes.
  • Open a Pull Request to my VS Code extension repository.

By "with these comments", I just meant to still work on my suggestions - just like you have been doing! Thanks for all your hard work!

@JoaoLira-br
Copy link
Author

I`m a bit undecided on the names for these two procedures. Do you think that these prefixes would be good for it, or are they too literal?
Screenshot from 2022-11-27 15-10-47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants