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

Patch reporter #911

Merged
merged 6 commits into from
Nov 9, 2021
Merged

Patch reporter #911

merged 6 commits into from
Nov 9, 2021

Conversation

m42e
Copy link
Contributor

@m42e m42e commented Oct 31, 2021

Solves #587
Hope this helps to get closet to V1.0 (#775)

This reporter is available as `Patches`. It generates a patch for each mutant.
@m42e
Copy link
Contributor Author

m42e commented Oct 31, 2021

Suggestion: use --git-project-root as base directory. What do you think? @stanislaw @AlexDenisov

@AlexDenisov
Copy link
Member

Thank you so much for bringing this up! This is certainly a great start!

Could you add some test cases for some mutation operators? In particular, the interesting operators are cxx_lshift_to_rshift, cxx_lt_to_ge, cxx_le_to_lt (at least).

The other important one is cxx_remove_void_call, but I don't think it's possible to get it right without actually parsing the source code.

Suggestion: use --git-project-root as base directory. What do you think?

What is the base directory in this context?

@m42e
Copy link
Contributor Author

m42e commented Oct 31, 2021

Ok, will do i can add some sample files, too.

Base path means the Path the patch should be relative to. For now it is absolute.

@m42e
Copy link
Contributor Author

m42e commented Oct 31, 2021

The test should use simply Google test or is there another way these check should be done?

@AlexDenisov
Copy link
Member

There are integration tests based on LLVM lit and filecheck under tests-lit directory, I'd recommend adding the tests there.

@AlexDenisov
Copy link
Member

Oh, and the git root sounds good as a base path!

@m42e
Copy link
Contributor Author

m42e commented Oct 31, 2021

Oh, and the git root sounds good as a base path!

I decided to use the git path as fallback for mull-cxx and added an extra command line parameter, as mull-runner does not have the git-project-dir option. Feel free to suggest otherwise. But I think it might be a bit misleading in the mull-runner context if the git-project-dir is not used otherwise.

@AlexDenisov
Copy link
Member

Just played around with it locally and it looks pretty good!
We'll need to mention somewhere that it only works for single-line changes (i.e. multiline patches won't work correctly) and that some mutators may produce invalid code (at least cxx_init_const and negate_mutator don't work properly).

All in all, looks good and I'm happy to merge unless you want to add something :)

@m42e
Copy link
Contributor Author

m42e commented Nov 7, 2021

Feel free to merge. Also feel free to send me examples of what’s not working an I will have a look at it.

@AlexDenisov
Copy link
Member

Also feel free to send me examples of what’s not working an I will have a look at it.

cxx_const_init doesn't work properly with the hello-world example.
remove_void_function produces incorrect diff for something like

void foo(...);
///
foo(a,
      b);

and I guess

void foo();
void bar();
///
foo(); bar();

will also be incorrect.

All in all, there is no straightforward way to handle this at the moment: we need to attach certain metadata to some mutation points.

@AlexDenisov AlexDenisov merged commit 9f2accc into mull-project:main Nov 9, 2021
@m42e
Copy link
Contributor Author

m42e commented Nov 9, 2021

@AlexDenisov I tried the cxx_init_const and I think it is actually the replacement information or the source location information that is not correct. It is also not displayed correctly using the Elements reporter.

The multiline does not work, that's sadly true, fix is here: #913

the third example works just fine.

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