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

Made CodeAssembler traits public #236

Merged
merged 3 commits into from
Dec 15, 2021
Merged

Conversation

Kixiron
Copy link
Contributor

@Kixiron Kixiron commented Dec 4, 2021

Closes #231

@wtfsck
Copy link
Member

wtfsck commented Dec 5, 2021

Copy link
Member

@wtfsck wtfsck left a comment

Choose a reason for hiding this comment

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

Build failed.

You've also accidentally committed some binary files.

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 6, 2021

The build error is from missing docs, do you want me to modify the generator to add the docs that go on the assembler methods to the traits (e.g. CodeAssember::mov()) or just ignore them for now?

@wtfsck
Copy link
Member

wtfsck commented Dec 6, 2021

Yes, you can disable all warnings about docs (eg. adding a #![docs(hidden)] will probably fix that)

I noticed you renamed the module. This will possibly cause problems with the docs. It's possible that all the generated methods will be shown first. See docs here where eg. the constructor is at the top of the file (after the module docs) and all the generated methods are at the end.

Is the order still the same or are all generated methods at the top now?

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 6, 2021

It looks to be the same in terms of method layout
image
image

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 6, 2021

I'm getting this warning from rustdoc but I'm not sure what version of rust it's from vs. what versions of rust you support, would you like me to fix it?

warning: lint `invalid_html_tags` has been renamed to `rustdoc::invalid_html_tags`
  --> iced-x86\src\lib.rs:13:9
   |
13 | #![warn(invalid_html_tags)]
   |         ^^^^^^^^^^^^^^^^^ help: use the new name: `rustdoc::invalid_html_tags`
   |
   = note: `#[warn(renamed_and_removed_lints)]` on by default

@wtfsck
Copy link
Member

wtfsck commented Dec 6, 2021

You can ignore that warning. We need to support rust 1.48.0 and it doesn't support the new rustdoc warning name.

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 6, 2021

I added trait documentation to the generator and ran it, but it seems to have changed a bunch of files from LF to CRLF, do you know how to fix that?

Edit: I think this is being caused by Environment.NewLine which is defaulting to \r\n since I'm on windows

@wtfsck
Copy link
Member

wtfsck commented Dec 6, 2021

Docs aren't needed, a simple doc(hidden) at the top should be enough.

Maybe you didn't enable the git option to use default line endings, I think it just checks out LF line endings when it should be checking out files with CRLF line endings. You should probably set core.autocrlf to true, see https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration . Reset the files and re-run the generator and it will hopefully work.

@Kixiron Kixiron requested a review from wtfsck December 12, 2021 00:24
@wtfsck
Copy link
Member

wtfsck commented Dec 12, 2021

Did it compile without any warnings/errors?

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 14, 2021

Clippy gives missing inline warnings for all of the trait methods

warning: missing `#[inline]` for a method
     --> iced-x86\src\code_asm\fn_asm_impl.rs:74231:2
      |
74231 | /     fn vsqrtpd(&mut self, op0: AsmRegisterYmm, op1: AsmMemoryOperand) -> Result<(), IcedError> {
74232 | |         let code = if op1.is_broadcast() {
74233 | |             Code::EVEX_Vsqrtpd_ymm_k1z_ymmm256b64
74234 | |         } else if self.instruction_prefer_vex() {
...     |
74239 | |         self.add_instr_with_state(Instruction::with2(code, op0.register(), op1.to_memory_operand(self.bitness()))?, op0.state().merge(op1...
74240 | |     }
      | |_____^
      |
      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items

A bunch of tests have redundant closures

warning: redundant closure
   --> iced-x86\src\formatter\nasm\tests\mod.rs:127:57
    |
127 |     formatter_test_nondec(64, "Nasm", "NonDec_MemMinimum", || create_memminimum());
    |                                                            ^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `create_memminimum`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

warning: `iced-x86` (lib test) generated 183 warnings

But my changes didn't cause these, what do you want me to do?

@wtfsck
Copy link
Member

wtfsck commented Dec 14, 2021

Try this (code_asm.rs)

+#[allow(missing_docs)]
+#[allow(clippy::missing_errors_doc)]
+pub mod asm_traits;
 mod code_asm_methods;
+#[allow(clippy::missing_inline_in_public_items)]
 mod fn_asm_impl;
 mod fn_asm_pub;
-mod fn_asm_traits;
 mod mem;
 mod op_state;
 mod reg;

Note that I removed doc(hidden) and added allow(missing_docs)

Copy link
Member

@wtfsck wtfsck left a comment

Choose a reason for hiding this comment

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

.

@Kixiron
Copy link
Contributor Author

Kixiron commented Dec 15, 2021

Fixed all of the clippy warnings, should be good to go

Copy link
Member

@wtfsck wtfsck left a comment

Choose a reason for hiding this comment

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

Can you undo the last commit? I fixed those earlier already, see #239

Assuming everything builds (after removing the last commit), I'll merge this PR! Thanks!

@wtfsck wtfsck merged commit dbfb63d into icedland:master Dec 15, 2021
@wtfsck
Copy link
Member

wtfsck commented Dec 15, 2021

Thanks, merged!

@Kixiron Kixiron deleted the export-asm-traits branch December 15, 2021 19:09
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.

Can't write code generically over CodeAssembler
2 participants