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

analyze: add function attrs for testing #942

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Conversation

spernsteiner
Copy link
Collaborator

Adds support for some function attributes that are useful for testing:

  • #[c2rust_analyze_test::fixed_signature]: Marks all pointers in the function signature as FIXED
  • #[c2rust_analyze_test::fail_analysis]: Forces an analysis failure for the function
  • #[c2rust_analyze_test::skip_rewrite]: Skips generating rewrites for the function

c2rust-analyze/src/main.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kkysen kkysen 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 add a simple example/test for this, too (such as a use case from #936)? Then this should be all good to go I think, besides my other small comments.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

#![feature(register_tool)] (rust-lang/rust#66079) seems fairly unstable, but I don't see another way of adding this functionality simply, so I think it's fine here.

AttrKind::DocComment(..) => continue,
};
let (a, b) = match &path.segments[..] {
&[ref a, ref b] => (a, b),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
&[ref a, ref b] => (a, b),
[a, b] => (a, b),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mostly avoid using the "match ergonomics" feature - it makes it less clear whether the match is binding by reference or by value, which is generally useful to know IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that clear from the & in the &path.segments[..]? The & and ref are basically inverses, right, so doing &[ref seems quite redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO &[ref a, ref b] is much more confusing than [a, b]. Match ergonomics are used all over the place, in most idiomatic Rust code and ours.

c2rust-analyze/src/util.rs Outdated Show resolved Hide resolved
@spernsteiner
Copy link
Collaborator Author

Can you add a simple example/test for this, too?

Added a basic test case in 2244cbe

@spernsteiner spernsteiner merged commit 1858046 into master Jun 6, 2023
Comment on lines +504 to +507
let lsig = match gacx.fn_sigs.get(&ldid.to_def_id()) {
Some(x) => x,
None => continue,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

@spernsteiner, I vaguely remember you answering this before, but I can't find the comment. Why do we continue here instead of panicking? When would the lookup fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure. I tried changing the continue to a panic!() just now and nothing broke in the tests (though only a few of those use fixed_signature).

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.

2 participants