search

sampsyo / bril

an educational compiler intermediate representation
https://capra.cs.cornell.edu/bril/
MIT License
579 stars 238 forks source link

Add full bril-rs feature set to rs2bril #315

Closed ajpal closed 8 months ago

ajpal commented 8 months ago

Similar to https://github.com/sampsyo/bril/pull/312, this PR adds the full feature set to rs2bril so that we can depend on both.

Pat-Lafon commented 8 months ago

Two quick questions:

ajpal commented 8 months ago

  • 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 commented 8 months ago

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 https://github.com/sampsyo/bril/blob/0eedeea2536d5b7ef09655a7c2be03ce5045447b/brilirs/Cargo.toml#L46-L47 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 commented 8 months ago

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

sampsyo commented 8 months ago

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

Pat-Lafon commented 8 months ago

LGTM

ajpal commented 8 months ago

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

Pat-Lafon commented 8 months ago

@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 commented 8 months ago

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