Closed GoogleCodeExporter closed 9 years ago
Solution in revision b016db026b30, please review.
Original comment by ssukkerd
on 11 Feb 2013 at 3:42
Here are the comments that cover everything up to revision 143cc0d5f0d3 in
Issue285 (as far as I can tell):
- Rename DaikonType to DaikonVarType
- Rename PartitionWrapper to PartitionInvWrapper
- Refactor the repetitive code in PartitionWrapper.populateInvariants() (you
can specify post/pre as an arg to the refactored function) e.g., eliminate
repetition that looks like this:
if (succEvent.getParent().compareTo(succPartition) == 0) {
daikonizer.addInstance(event.getPostEventState());
}
- Move the State.determineType() method into DaikonType and make it into a
static method there, something like "stringToDaikonVarType". It makes more
sense to have it there -- if you add a new DaikonVarType then you'll remember
to update the parsing method.
- SynDaikonizer.addInstance() is somewhat confusing. There are two logical
states to SynDaikonizer -- either the internal daikonizer instance has been
initialized, or it hasn't been. Create a method for each of these cases, and
call them appropriately from addInstance.
- A better name for SynDaikonizer.getDaikonInvariants is
getDaikonEnterInvariants.
- You must refactor Daikonizer to suit your needs. For example, instead of
this: "daikonizer.addValues(record, record);" , define a new method in
Daikonizer that does this internally, and takes a single vector with vals. I
think there are other, similar, refactorings that you can make to simplify
SynDaikonizer.
- All of StateTests is really just testing the parsing of DaikonType. After you
move the parsing into DaikonVarType, you can rename these test to be
DaikonVarTypeTests.
- SynDaikonizerTests only tests that some invariants are generated (could be 0
actually) -- it runs daikon and prints out the inferred invariants. Instead,
change this to check that you've gotten the _right_ invariants from Daikon.
- You can comment out the code in DirectDaikonizer to make the project compile.
Just add a TODO at the top explaining that it is currently broken.
Original comment by bestchai
on 12 Feb 2013 at 11:22
I did the following suggestions:
- Rename DaikonType to DaikonVarType
- Rename PartitionWrapper to PartitionInvWrapper
- Move the State.determineType() method into DaikonType and make it into a
static method there, something like "stringToDaikonVarType". It makes more
sense to have it there -- if you add a new DaikonVarType then you'll remember
to update the parsing method.
- SynDaikonizer.addInstance() is somewhat confusing. There are two logical
states to SynDaikonizer -- either the internal daikonizer instance has been
initialized, or it hasn't been. Create a method for each of these cases, and
call them appropriately from addInstance.
- A better name for SynDaikonizer.getDaikonInvariants is
getDaikonEnterInvariants.
- All of StateTests is really just testing the parsing of DaikonType. After you
move the parsing into DaikonVarType, you can rename these test to be
DaikonVarTypeTests.
- You can comment out the code in DirectDaikonizer to make the project compile.
Just add a TODO at the top explaining that it is currently broken.
Below are the suggestions that I didn't follow and the reasons why:
- Refactor the repetitive code in PartitionWrapper.populateInvariants() (you
can specify post/pre as an arg to the refactored function) e.g., eliminate
repetition that looks like this:
if (succEvent.getParent().compareTo(succPartition) == 0) {
daikonizer.addInstance(event.getPostEventState());
}
> If I created a new method for it, I would have to pass in a bunch of
arguments to that method. It would look ugly and wouldn't hep reduce repetition
that much (it's just a 2-line code).
- You must refactor Daikonizer to suit your needs. For example, instead of
this: "daikonizer.addValues(record, record);" , define a new method in
Daikonizer that does this internally, and takes a single vector with vals. I
think there are other, similar, refactorings that you can make to simplify
SynDaikonizer.
> This should be a separate issue. There is so much refactoring I could do with
Daikonizer. I haven't got into that because it wasn't a high priority task.
- SynDaikonizerTests only tests that some invariants are generated (could be 0
actually) -- it runs daikon and prints out the inferred invariants. Instead,
change this to check that you've gotten the _right_ invariants from Daikon.
> With a small set of data, you can't know in advance what kind of invariants
Daikon would mine, even for data with obvious relationship. For example, I have
a set of x,y data that the invariant should be y=x+1, but Daikon doesn't mine
that. It only mines y > x, 0 <= x <= 63 or something along those lines. Also, I
don't see the point of checking that Daikon returns the _right_ invariants. I'm
not testing Daikon. I'm only testing that the SynDaikonizer aligns values to
variables correctly.
Original comment by ssukkerd
on 14 Feb 2013 at 1:06
I'm okay with you not refactoring populateInvariants.
Refactoring Daikonizer could be a separate issue, but the problem is that
refactorings that are obvious to us now will not be so obvious later. Can you
create an issue for this and document some of refactoring todos that you
already know about?
For SynDaikonizerTests I have two comments:
1. I don't see how the existing code tests anything. Since you are not
asserting anything the only thing you are testing is that there was no
exception thrown.
2. The idea behind testing the invariants is to sanity check that the
invariants you get from Daikon are what you expect. If you're getting "y > x,
0 <= x <= 63", but expect "y=x+1" then something is definitely wrong. Either
the problem is in the Daikonizer, or in SynDaikonizer, or in the parameters
passed to Daikon, or in our understanding of how Daikon works. Regardless, this
is good! You need to get to the bottom of this and figure out why it's not
giving you the right invariant and change things so that it does give you the
right invariant. Then, you can add the assert for "y=x+1". In other words,
ignoring the problem you've found just delays the solution until a later point.
I would rather you worked out the kinks with Daikon and its interface in this
issue, since that's what Issue 284 is all about -- mining (expected)
invariants. But, you can also make this into a separate issue. It doesn't
matter -- you'll have to debug this eventually to have a working system. It's
your choice if you want to solve it now, or in a separate issue.
Original comment by bestchai
on 14 Feb 2013 at 2:32
Solution (except refactoring Daikonizer, which will go into a separate issue)
in revision 19f632478238, please review.
Original comment by ssukkerd
on 19 Feb 2013 at 5:07
Merged into default with revision ef675d4d0691
Original comment by bestchai
on 19 Feb 2013 at 7:19
Original issue reported on code.google.com by
ssukkerd
on 7 Feb 2013 at 12:42