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

Add full bril-rs feature set to rs2bril #315

Merged
merged 6 commits into from
Apr 5, 2024

Conversation

ajpal
Copy link
Contributor

@ajpal ajpal commented Mar 27, 2024

Similar to #312, this PR adds the full feature set to rs2bril so that we can depend on both.

@ajpal ajpal changed the title [rs2bril] Turn on import feature Add full bril-rs feature set to rs2bril Mar 27, 2024
@Pat-Lafon
Copy link
Contributor

Two quick questions:

  • what happens if you don't include some features that rs2bril will never use/consider like ssa, speculate?
  • Instead of adding some features as default and continuing feature creep spread. Can we just enable them as feature flags like import? (Ideally most of the rust tools should do this though when I explored this, lalrpop had an open missing feature I would have needed.)

@ajpal
Copy link
Contributor Author

ajpal commented Mar 27, 2024

  • what happens if you don't include some features that rs2bril will never use/consider like ssa, speculate?
    I think this works! I updated to only add import
  • Instead of adding some features as default and continuing feature creep spread. Can we just enable them as feature flags like import? (Ideally most of the rust tools should do this though when I explored this, lalrpop had an open missing feature I would have needed.)
    Sorry, I'm not really sure what that means / how to do it? If the import feature is enabled, that changes the fields that need to be included in the Program struct. Is there a way to do that other than making import always enabled for rs2bril?

@Pat-Lafon
Copy link
Contributor

Pat-Lafon commented Mar 27, 2024

Sorry, I'm not really sure what that means / how to do it? If the import feature is enabled, that changes the fields that need to be included in the Program struct. Is there a way to do that other than making import always enabled for rs2bril?

Something like

bril/brilirs/Cargo.toml

Lines 46 to 47 in 0eedeea

[features]
completions = ["clap_complete"]
but instead of enabling an optional dependency, enable the bril-rs feature flag for import and then feature gate that one change that import requires for the Program struct

@sampsyo
Copy link
Owner

sampsyo commented Mar 29, 2024

@Pat-Lafon's useful comments aside, it looks like cargo fmt is suggesting a minuscule change here!

@sampsyo
Copy link
Owner

sampsyo commented Mar 30, 2024

Thanks, @ajpal! @Pat-Lafon, if this looks good to you now, I'm down to merge.

@Pat-Lafon
Copy link
Contributor

LGTM

@ajpal
Copy link
Contributor Author

ajpal commented Apr 2, 2024

@sampsyo @Pat-Lafon Is this ready to merge then? Lmk if there are any other changes you'd like me to make!

@Pat-Lafon
Copy link
Contributor

@sampsyo @Pat-Lafon Is this ready to merge then? Lmk if there are any other changes you'd like me to make!

It should be good, just probably low on Adrian's busy queue.

@sampsyo
Copy link
Owner

sampsyo commented Apr 5, 2024

I was on vacation this week. 😃 Looks good though; thank you again!

@sampsyo sampsyo merged commit 57877b1 into sampsyo:main Apr 5, 2024
16 checks passed
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.

3 participants