machinst x64: refactor to use types::[type] everywhere#2088
machinst x64: refactor to use types::[type] everywhere#2088abrown merged 1 commit intobytecodealliance:mainfrom
Conversation
Subscribe to Label Actioncc @bnjbvr DetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:x64"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
cfallin
left a comment
There was a problem hiding this comment.
Sure, this seems reasonable. @julian-seward1 or @bnjbvr may have different opinions here, but (i) many of the locations this refactor touches were inconsistent in the first place (there were some ir::types::X as well as bare X instances), and (ii) IMHO, a little extra explicitness is good when it's not so much to be cumbersome. So, +1 from me. Thanks!
|
cc: @jlb6740 |
|
@abrown Thanks, agree with everything said. As @cfallin said .. the use of types:: prefix is inconsistent so this does improve on that. As you said it does increase verbosity which I think can sometimes reduce readability when it is too redundant, but maybe this allows ide's and the programming to have to parse and deal with less ambiguity. So pros and cons .. I personally wouldn't mind either way. Interested in what others prefer. |
This change is a pure refactoring--no change to functionality. It removes `use crate::ir::types::*` imports and uses instead `types::I32`, e.g., throughout the x64 code. Though it increases code verbosity, this change makes it more clear where the type identifiers come from (they are generated by `cranelif-codegen-meta` so without a prefix it is difficult to find their origin), avoids IDE confusion (e.g. CLion flags the un-prefixed identifiers as errors), and avoids importing unwanted identifiers into the namespace.
|
I plan on merging this before it gets too out-of-date (especially since #2100 removes the x64 CI check--I ran the x64 tests locally on the rebased version and the unrebased commit already passed the x64 CI check). |
This change is a pure refactoring--no change to functionality. It removes
use crate::ir::types::*imports and uses insteadtypes::I32, e.g., throughout the x64 code. Though it increases code verbosity, this change makes it more clear where the type identifiers come from (they are generated bycranelif-codegen-metaso without a prefix it is difficult to find their origin), avoids IDE confusion (e.g. CLion flags the un-prefixed identifiers as errors), and avoids importing unwanted identifiers into the namespace.