Refactor code related to block construction #571
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ArsenHD
force-pushed
the
minor-codegen-refactoring
branch
from
141d7cb to
cbc5329
Compare
EgorkaKulikov
approved these changes
Jul 28, 2022
sofurihafe
approved these changes
Jul 29, 2022
Code generation relies on CgContext for storing information about the currently available variables, imports, statements, etc. The context also stores info about code block currently being generated (currentBlock). It is possible to collect statements into some list and then update the currentBlock with them, but it is discouraged, because this way it is very easy to occacionally lose some of the statements. That's because some statements will go to the currentBlock, but then will be overriden on update from the new list of statements. Hence, it is recommended to always work with the statements using currentBlock and DSL methods that also use it. Such methods can be found in CgStatementConstructor.kt, see method `ifStatement` for example. This PR makes more use of currentBlock instead of using separate lists. That is done in order to prevent possible future bugs.
This idea of this refactoring is the same as in the previous commit. Instead of collecting statements into some lists we now collect them right into the currentBlock.
ArsenHD
force-pushed
the
minor-codegen-refactoring
branch
from
1402e38 to
1f5b985
Compare
The problem was in the parameterized tests. There were multiple bugs: wrong class ids (class id's name must not include generic parameters, but they did), wrong method id for Arguments.arguments() method (its argument must be vararg, but it wasn't). Also, when a method accepts a vararg and is called via reflection, an array of vararg arguments should be additionally wrapped into an array of Objects (e.g. new Object[]{new Integer[]{1, 2, 3}}). However, when calling this method without reflection there must not be any such wrappers.
- Make Arguments#arguments method id static. This allowed us to call this method without reflection. Previously we used reflection, because the method id was non-static by mistake. - Construct an array of arguments for Arguments#arguments call. This way we don't have to worry about multiple cases of vararg method calls, but we should think about them later.
…h array allocation and initialization
ArsenHD
force-pushed
the
minor-codegen-refactoring
branch
from
1f5b985 to
824c543
Compare
Earlier type parameters were hardcoded in the class name, simple name, etc. Then they were removed from names completely, but that lead to compilation errors for Kotlin, where type parameters must always be specified. So, this commit adds type parameters, but instead of writing them into names, we add them as a separate property 'typeParameters' of BuiltinClassId.
ArsenHD
force-pushed
the
minor-codegen-refactoring
branch
from
824c543 to
ce9d3e0
Compare
denis-fokin
pushed a commit
that referenced
this pull request
Aug 18, 2022
* Refactor code related to block construction
Code generation relies on CgContext for storing information about the currently available variables, imports, statements, etc. The context also stores info about code block currently being generated (currentBlock).
It is possible to collect statements into some list and then update the currentBlock with them, but it is discouraged, because this way it is very easy to occacionally lose some of the statements. That's because some statements will go to the currentBlock, but then will be overriden on update from the new list of statements.
Hence, it is recommended to always work with the statements using currentBlock and DSL methods that also use it. Such methods can be found in CgStatementConstructor.kt, see method `ifStatement` for example.
This PR makes more use of currentBlock instead of using separate lists. That is done in order to prevent possible future bugs.
* Refactor data provider method construction
This idea of this refactoring is the same as in the previous commit. Instead of collecting statements into some lists we now collect them right into the currentBlock.
* Fix ClassWithEnumTest.testOrdinal test
The problem was in the parameterized tests. There were multiple bugs: wrong class ids (class id's name must not include generic parameters, but they did), wrong method id for Arguments.arguments() method (its argument must be vararg, but it wasn't). Also, when a method accepts a vararg and is called via reflection, an array of vararg arguments should be additionally wrapped into an array of Objects (e.g. new Object[]{new Integer[]{1, 2, 3}}). However, when calling this method without reflection there must not be any such wrappers.
* Fix Arguments#arguments method call
- Make Arguments#arguments method id static. This allowed us to call this method without reflection. Previously we used reflection, because the method id was non-static by mistake.
- Construct an array of arguments for Arguments#arguments call. This way we don't have to worry about multiple cases of vararg method calls, but we should think about them later.
* Limit the maximum length of an array initializer + use guards for both array allocation and initialization
* Rework type parameters for some class ids
Earlier type parameters were hardcoded in the class name, simple name, etc. Then they were removed from names completely, but that lead to compilation errors for Kotlin, where type parameters must always be specified. So, this commit adds type parameters, but instead of writing them into names, we add them as a separate property 'typeParameters' of BuiltinClassId.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Code generation relies on CgContext for storing information about the currently available variables, imports, statements, etc. The context also stores info about code block currently being generated (currentBlock).
It is possible to collect statements into some list and then update the currentBlock with them, but it is discouraged, because this way it is very easy to occacionally lose some of the statements. That's because some statements will go to the currentBlock, but then will be overriden on update from the new list of statements.
Hence, it is recommended to always work with the statements using currentBlock and DSL methods that also use it. Such methods can be found in CgStatementConstructor.kt, see method
ifStatementfor example.This PR makes more use of currentBlock instead of using separate lists. That is done in order to prevent possible future bugs.
Type of Change
How Has This Been Tested?
Automated Testing
The whole code generation pipeline is used to check the changes.
Given that most of the changes are related to the deep equals code generation,
the most useful test would be
org.utbot.examples.codegen.deepequals.DeepEqualsTest.Manual Scenario
I wrote a method that worked with multidimensional floating point arrays in the IntelliJ with UtBot plugin and the result of code generation was the same as before changes, i.e. refactoring did not break such an example.
Checklist (remove irrelevant options):