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.

Sign up for GitHub

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

Jump to bottom

Extracting Depedent functions #20

Merged
merged 2 commits into from
May 31, 2024
Merged

Extracting Depedent functions #20

merged 2 commits into from
May 31, 2024

Conversation

snapdgn
Copy link

@snapdgn snapdgn commented May 22, 2024 •
edited
Loading

Fixes: ThinkOpenly/sail-riscv#17
This currently extracts the dependent function(not exactly: it extracts all the function defs) into a new hashtable.

@snapdgn snapdgn force-pushed the depedent_functions branch 2 times, most recently from 05eba2f to 977fae3 Compare May 23, 2024 09:28
@snapdgn snapdgn marked this pull request as ready for review May 23, 2024 09:35
@snapdgn snapdgn force-pushed the depedent_functions branch from 977fae3 to e386b44 Compare May 23, 2024 09:49
ThinkOpenly
ThinkOpenly requested changes May 23, 2024
View reviewed changes
Copy link
Owner

@ThinkOpenly ThinkOpenly 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! I have a couple of minor comments inline.

src/sail_json_backend/json.ml Outdated
| Pat_exp (P_aux (P_tuple pl, _), e)
| Pat_when (P_aux (P_tuple pl, _), e, _) ->
debug_print ("id_of_dependent: " ^ id);
let operandl = List.concat (List.map string_list_of_pat pl) in
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this line is needed. operandl is not used in the subsequent code in this stanza.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, sorry totally missed it.

src/sail_json_backend/json.ml Outdated
@@ -659,5 +669,12 @@ let defs { defs; _ } =
print_endline " \"extensions\": [";
let extension_list = Hashtbl.fold (fun k v accum -> v :: accum) extensions [] in
print_endline (String.concat ",\n" (List.sort_uniq String.compare (List.concat extension_list)));
print_endline " ],";

print_endline " \"dependent_functions\": [";
Copy link
Owner

Choose a reason for hiding this comment

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

Since the set of functions here are not all dependencies, let's change the key here to something more generic like "functions" (in some ways, these are "independent functions", but I think "functions" is nicely generic).

Copy link
Author

Choose a reason for hiding this comment

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

what about the hashtable name for it, should i rename that too? That would conflict with the already defined 'functions' table(which hosts the 'function clause execute'). Should that be renamed to something else too?

Copy link
Owner

Choose a reason for hiding this comment

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

That would conflict with the already defined 'functions' table
I certainly overlooked that. I'm really tempted to rename the older table from "functions" to, say, "executes" (not terribly excited by that name, though).

If you are willing, could you create a first patch which does that rename, then add your changes as separate patch(es) on top of that?

Copy link
Author

Choose a reason for hiding this comment

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

Just to make sure i understand correctly, I should just rename print_endline " \"dependent_functions\": ["; at first. And then work on renaming things like prior functions fields to let's say execute etc..?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the confusion. I suggest creating a "first patch" which renames the existing "functions" Hashtbl to something more apt, like "executes", or "semantics", or "rtl". (As you can see, I'm having trouble coming up with a perfect name.)

Then, you can have a second patch (or more, if needed) that adds your new "functions" Hashtbl that collects all of the other function bodies (without a name collision on "functions").

Copy link
Author

Choose a reason for hiding this comment

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

got it, thanks for the clarification.

@snapdgn snapdgn force-pushed the depedent_functions branch from e386b44 to 7a57196 Compare May 24, 2024 16:13
@snapdgn
Copy link
Author

snapdgn commented May 24, 2024

Let me know if this looks any good. @ThinkOpenly

@snapdgn snapdgn force-pushed the depedent_functions branch 2 times, most recently from 39d598d to 608c7dd Compare May 24, 2024 16:59
@ThinkOpenly
Copy link
Owner

Let me know if this looks any good.

I like that there is a single commit that renames "functions" to "executes". Are you planning to squash together the rest of the patches? I was expecting the final set of commits to look something like:

  1. rename "functions" to "executes"
  2. extract function bodies to new Hashtbl "functions"

@snapdgn snapdgn force-pushed the depedent_functions branch from 608c7dd to b845aa1 Compare May 30, 2024 04:24
@snapdgn
Copy link
Author

snapdgn commented May 30, 2024 •
edited
Loading

Apologies for the delay, it slipped my mind entirely.
I couldn't squash it into two commits because i couldn't directly create a hashtable named functions (since it was already taken). I've squashed some of previous ones though.
Let me know if the above seems good.

@ThinkOpenly
Copy link
Owner

Apologies for the delay, it slipped my mind entirely. I couldn't squash it into two commits because i couldn't directly create a hashtable named functions (since it was already taken). I've squashed some of previous ones though. Let me know if the above seems good.

Here's what I was thinking. You currently have 4 commits:

  1. [JSON]: Implement support for optional operand
    Actually, should this be here? I think you need to rebase.
  2. [JSON]: extract function bodies to a new hashtable depedendent_functions
  3. rename functions to executes
  4. rename dependent_functions to functions

I propose rebasing/reordering/squashing to produce 2 commits:

  1. rename functions to executes
  2. squash the following into a single commit:
    1. extract function bodies to a new hashtable
    2. rename dependent_functions to functions

@snapdgn snapdgn force-pushed the depedent_functions branch from b845aa1 to 06c8804 Compare May 31, 2024 11:06
@snapdgn snapdgn force-pushed the depedent_functions branch from 06c8804 to 6c7c9d1 Compare May 31, 2024 13:09
@snapdgn
Copy link
Author

snapdgn commented May 31, 2024

  • rebased against upstream.
  • reordered & squashed as suggested.

@ThinkOpenly ThinkOpenly merged commit 4aa4024 into ThinkOpenly:json May 31, 2024
0 of 2 checks passed
@snapdgn snapdgn deleted the depedent_functions branch May 31, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThinkOpenly ThinkOpenly ThinkOpenly approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[JSON] extract dependent functions
2 participants
@snapdgn @ThinkOpenly