-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add new IcfgBuilder #690
Add new IcfgBuilder #690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work! I did not have a detailed look yet, but here are already some more general comments.
trunk/examples/settings/automizer/ForwardPredicates_IcfgBuilder.epf
Outdated
Show resolved
Hide resolved
trunk/examples/Backtranslation/regression/bpl/Pointer.errorpath
Outdated
Show resolved
Hide resolved
...informatik/ultimate/plugins/generator/icfgbuilder/preferences/IcfgPreferenceInitializer.java
Show resolved
Hide resolved
...c/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/WeakestPrecondition.java
Show resolved
Hide resolved
trunk/examples/settings/automizer/ForwardPredicates_IcfgBuilder.epf
Outdated
Show resolved
Hide resolved
...e/uni_freiburg/informatik/ultimate/plugins/generator/rcfgbuilder/cfg/LargeBlockEncoding.java
Outdated
Show resolved
Hide resolved
105740d
to
d00bf86
Compare
I just synced the |
...er/src/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/cfg/CfgBuilder.java
Outdated
Show resolved
Hide resolved
...er/src/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/cfg/CfgBuilder.java
Outdated
Show resolved
Hide resolved
firstStatement = | ||
new ReturnStatement(mBoogieDeclarations.getProcImplementation().get(procName).getLocation()); | ||
} else { | ||
firstStatement = body.getBlock()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the RCFGBuilder
firstStatement
is always set to body.getBlock()[0]
, why is this different here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the RCFGBuilder
body.getBlock()
could never be empty, because the BoogiePreprocessor
would insert a return statement in that case. With the setting that disables the UnstructureCode
class from the BoogiePreprocessor
body.getBlock()
can now be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does this belong to the CfgBuilder
? Wouldn't it be better to move it into its own Preprocessor that always runs independant of UnstructureCode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we even need this statement here? Is it possible to create a location without a statement?
...er/src/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/cfg/CfgBuilder.java
Show resolved
Hide resolved
5470c7c
to
7499e73
Compare
Some info (not enough time to think about your post): The new IcfgBuilder alredy makes sure that labels get their own IcfgLocation. |
bc983fe
to
a20ad9d
Compare
a20ad9d
to
d19095a
Compare
d19095a
to
b0202f8
Compare
Otherwise the StepInfo is reset, since mutable sets are used.
d1e40c5
to
9107825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good so far. The branch has already been correctly adapted to our Java 21 and Eclipse migration and can therefore be merged without any issues.
However, there are still a few small things that need to be improved. For example, in the comments and the license agreement in the headers of ICFG source code files, the word RCFGBuilder should be replaced consistently with ICFGBuilder.
...rc/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/IcfgBacktranslator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NiklasKult, @Heizmann and @schuessf for working on this!
I had a look at the changes and did not find any major issues. Due to the time constraints, I was not able to review in-depth, but since others have done so, and our latest evaluations have shown no major issues (only a slight increase in the number of locations) I have no objection to merging 👍
...rg/informatik/ultimate/plugins/analysis/abstractinterpretationv2/AbstractInterpretation.java
Outdated
Show resolved
Hide resolved
...rc/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/IcfgBacktranslator.java
Show resolved
Hide resolved
|
||
translateProgramExecution(final IProgramExecution<IIcfgTransition<IcfgLocation>, Term> programExecution) { | ||
if (!(programExecution instanceof IcfgProgramExecution)) { | ||
throw new IllegalArgumentException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: add an error message
...er/src/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/cfg/CfgBuilder.java
Show resolved
Hide resolved
...er/src/de/uni_freiburg/informatik/ultimate/plugins/generator/icfgbuilder/cfg/CfgBuilder.java
Outdated
Show resolved
Hide resolved
@@ -89,7 +89,7 @@ public class BoogieIcfgContainer extends ModernAnnotations implements IIcfg<Boog | |||
* Note: This map is only used during construction! | |||
* | |||
*/ | |||
final Map<String, BoogieIcfgLocation> mFinalNode; | |||
public final Map<String, BoogieIcfgLocation> mFinalNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to make this public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, as this is also used by the IcfgBuilder plugin. Of course, it would be more optimal to move it to some library, but we can do this, if we remove RCFGBuilder in the future.
I also had a quick look at the RCFGBuilder/ICFGBuilder diff by @schuessf and did not find any issues there either. |
I will try to address these minor issues and merge this later. |
Commits for switching from RCFGBuilder to IcfgBuilder are missing in this branch. But Frank convinced me to already start a pull request.