Issue 47 by pmbittner · Pull Request #50 · VariantSync/DiffDetective

pmbittner · 2022-09-23T14:36:56Z

Fixes for two parser bugs documented in issue #47 . Documentation is still missing but should be added.

…ncy + remove deprecated assertSemanticConsistency

ibbem

Did you notice the failing test in CI?
DiffNode.assertConsistency fails (because of a missing feature mapping) for LineGraphTest.idempotentReadWrite() with file src/test/resources/line_graph/vulkan_structs.hpp\$\$\$fd641ac85c6170c34845db5e345d3bf9cedce8d7.lg.
This test case should be adjusted somehow.

For example `#if defined()` was accepted.

In general, `HashMap`s are not deterministic because the default implementation of the hash function might not be deterministic (as in, for example, openjdk 18.0.2). This matters in this case because it's incorrect to replace '>' before '>='.

According to C11, constant expressions (used in macro conditions) can contain all operators available in normal code except assignment, increment, decrement, function-call, or comma operators. (§ 6.6 paragraph 3)

An example where this now works is `#ifdef ifndef`.

Previously, the macro `#if defined A` was not parsed correctly because first the space was removed and afterwards it was tried to replace the `defined` operator.

Note: This changes the abstracted names of macro calls in an backwards incompatible way.

ibbem · 2022-10-18T19:18:36Z

I just fixed the above mentioned parser issues. During coding/testing I found some more issues, some of them are already fixed. (Test cases follow later because I based them on #54 for easier testing.)

There are still unsolved problems:

The C11 standard allows character literals besides integers which are currently not handled at all.
The bracket replacement is more a heuristic than a robust algorithm, for example #if B - (C || D) is not parsed correctly.

@TheBormann You can try to merge this again, I hope this fixes some issues for you.

pmbittner · 2022-10-24T09:24:35Z

Hi @ibbem ,

thank you very much for your good review! I finally answered to your comments. Given that your upgrade to JUnit5 is now merged into develop and that you already fixed some comments here, could you

merge develop,
resolve all conversations above that your commits already addresses, and
take care of the concerns raised in the other comments? I agree with most of them.

Thank you very much.

pmbittner · 2022-10-24T09:28:55Z

Oops, I confused the other two pull-requests but the pull-request for JUnit5 is going to merged soon. :)

pmbittner · 2022-10-25T16:29:14Z

PR #54 is now merged.

ibbem · 2022-11-02T19:54:51Z

I replaced your tests with the test suite I wrote and moved the above mentioned problems into wontfix tests.

The test cases which was failing actually indicated a problem in the DiffNode.fromID function: It can't assign a meaningful feature mapping to the created node. As this function doesn't guarantee that it recovers all information I just used True to satisfy assertConsistency.

ibbem · 2022-11-02T20:18:38Z

@pmbittner I don't know why, but I'm unable to request a review from you. So I just inform you that I finished working on this PR.

pmbittner · 2022-11-03T07:51:40Z

Thank you for all the fixes! The change to using True in DiffNode.fromID makes sense as it is the neutral element for presence conditions. However, please use org.variantsync.diffdetective.util.fide.FixTrueFalse.True here. It is actually the same value but we use it as a constant across the entire project because there are some bugs in FeatureIDE handling True and False correctly upon SAT-Solving. That way, we have a consistent treatment of True and False across DiffDetective until the bug is fixed or until we migrate to FeatJar.

pmbittner

Changes look great! Only minor comments. Thank you very much! :)

You could not request a review from me because I am the author of the pull-request. 😅

There are many methods that just return garbage data and it's not documented that they may be unsupported. This also makes the interface for `DiffTree.fromPatch` more intuitive (I initially thought that it would take the diff from `PatchReference.getDiff`).

CommitDiffResult is not a good place to have such methods because its unexpected to have such convenience methods in the error handling type. Also, there are already many such convenience methods in `GitDiffer` most prominently `createCommitDiff` which does almost exactly what the moved method does, except it expects a `RevCommit` instead if a hash.

ibbem · 2022-11-03T10:50:45Z

@pmbittner All the above mentioned issues are fixed now.

pmbittner · 2022-11-03T14:06:41Z

Now only you have to approve the changes because initially you were the reviewer. ;) Afterwards, we can merge.

ibbem

From my point of view, this is good to go.

pmbittner added 7 commits September 16, 2022 12:35

fix: remove brackets around single words in boolean abstraction

1bfa44d

fix: disallow pure whitespace formulas

e621d82

fix: check that if and elif always have a formula upon assertConsiste…

f9ce4d8

…ncy + remove deprecated assertSemanticConsistency

feat: convencience constructors for commits and patches from repos

9593879

feat: instance of GitPatch that only serves as a reference

6e89cbf

feat: convenience constructor for DatasetDescription

30758dc

feat: test case covering the bug found by TheBormann

9533728

pmbittner self-assigned this Sep 23, 2022

pmbittner linked an issue Sep 23, 2022 that may be closed by this pull request

Brackets in variable names upon boolean abstraction #47

Closed

pmbittner changed the base branch from main to develop September 23, 2022 14:38

Merge branch 'develop' into issue47

f8258dc

pmbittner requested a review from ibbem October 6, 2022 11:25

pmbittner added the bug Something isn't working label Oct 6, 2022

ibbem requested changes Oct 9, 2022

View reviewed changes

ibbem added 7 commits October 18, 2022 21:05

Do not accept any empty annotations

b4389b2

For example `#if defined()` was accepted.

Make BooleanAbstraction deterministic

5e47940

In general, `HashMap`s are not deterministic because the default implementation of the hash function might not be deterministic (as in, for example, openjdk 18.0.2). This matters in this case because it's incorrect to replace '>' before '>='.

Add missing operators in BooleanAbstraction

391ece1

According to C11, constant expressions (used in macro conditions) can contain all operators available in normal code except assignment, increment, decrement, function-call, or comma operators. (§ 6.6 paragraph 3)

Do not get confused by macros with the name "ifndef"

e0124a3

An example where this now works is `#ifdef ifndef`.

Correctly replace the defined operator

2a21846

Previously, the macro `#if defined A` was not parsed correctly because first the space was removed and afterwards it was tried to replace the `defined` operator.

Replace parentheses around already abstracted formulas

36bf3e0

Note: This changes the abstracted names of macro calls in an backwards incompatible way.

Do not match C-style comments greedily

608c5f4

pmbittner added the bm_work ibbem is paid for working on this label Oct 25, 2022

Check for the absence of feature mappings

1d15d54

ibbem force-pushed the issue47 branch 2 times, most recently from 6380363 to 0388513 Compare November 2, 2022 19:48

pmbittner commented Nov 3, 2022

View reviewed changes

ibbem added 7 commits November 3, 2022 11:25

Fix two resource leaks (RevWalk has to be closed)

d6e69a5

Merge branch 'VariationSync/develop' into issue47

4199b47

Fix the annotation parser for unary minus and plus

67abb8e

Replace tests for parsing conditional macros

e52b2cd

Use True as default feature mapping for DiffNode.fromID

88a4faa

ibbem force-pushed the issue47 branch from 0388513 to 88a4faa Compare November 3, 2022 10:44

ibbem self-requested a review November 3, 2022 14:52

ibbem approved these changes Nov 3, 2022

View reviewed changes

pmbittner merged commit 93f836e into develop Nov 3, 2022

ibbem mentioned this pull request Nov 8, 2022

Implemention of variation trees #59

Merged

ibbem deleted the issue47 branch November 15, 2022 16:36

ibbem mentioned this pull request Feb 20, 2023

Integration of LB Thesis #63

Closed

Conversation

pmbittner commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibbem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibbem commented Oct 18, 2022

Uh oh!

pmbittner commented Oct 24, 2022

Uh oh!

pmbittner commented Oct 24, 2022

Uh oh!

pmbittner commented Oct 25, 2022

Uh oh!

ibbem commented Nov 2, 2022

Uh oh!

ibbem commented Nov 2, 2022

Uh oh!

pmbittner commented Nov 3, 2022

Uh oh!

pmbittner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibbem commented Nov 3, 2022

Uh oh!

pmbittner commented Nov 3, 2022

Uh oh!

ibbem left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pmbittner commented Sep 23, 2022 •

edited

Loading