kscience/kmath
9
0
Fork 1
Code Issues Pull Requests 6 Packages Projects Releases 16 Wiki Activity

ASM Bytecode Generation to unwrap Expressions of adv-expr API #94

Merged
CommanderTvis merged 44 commits from adv-expr into adv-expr 2020-06-13 21:07:15 +03:00
Conversation 0 Commits 44 Files Changed 16 +1010 -129
CommanderTvis commented 2020-06-05 18:08:43 +03:00 (Migrated from github.com)
Copy Link
No description provided.
altavir (Migrated from github.com) requested changes 2020-06-07 20:05:03 +03:00
altavir (Migrated from github.com) left a comment
Copy Link

I think that the main problem is the exposed design. The AsmGenerationContext should be made private or internal and hidden from the user. Also I think that produced entities should inherid Expression so this functionality is compatible with functional implementation.

I think that the main problem is the exposed design. The `AsmGenerationContext` should be made private or internal and hidden from the user. Also I think that produced entities should inherid `Expression` so this functionality is compatible with functional implementation.
kmath-asm/build.gradle.kts
altavir (Migrated from github.com) commented 2020-06-07 19:12:26 +03:00
Copy Link

Probably should be implementation

Probably should be implementation
kmath-asm/src/main/kotlin/scientifik/kmath/expressions/AsmExpressions.kt
altavir (Migrated from github.com) commented 2020-06-07 19:15:24 +03:00
Copy Link

Add suppress for unused parameters and documentation.

Add suppress for unused parameters and documentation.
altavir (Migrated from github.com) commented 2020-06-07 20:02:37 +03:00
Copy Link

Space is not actually used.

Space is not actually used.
altavir (Migrated from github.com) commented 2020-06-07 20:02:44 +03:00
Copy Link

Not used

Not used
altavir (Migrated from github.com) reviewed 2020-06-13 19:23:22 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/expressions/FunctionalExpressions.kt
@ -0,0 +1,139 @@
package scientifik.kmath.expressions
altavir (Migrated from github.com) commented 2020-06-13 19:23:22 +03:00
Copy Link

The rename is meaningless since there is no class with that name. FunctionalExpressionAlgebra is acceptable though.

The rename is meaningless since there is no class with that name. FunctionalExpressionAlgebra is acceptable though.
altavir (Migrated from github.com) reviewed 2020-06-13 19:25:28 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/expressions/Builders.kt
@ -0,0 +1,23 @@
package scientifik.kmath.expressions
altavir (Migrated from github.com) commented 2020-06-13 19:25:28 +03:00
Copy Link

There is no class called Builders. I suggest starting the name with lower case letter.

There is no class called `Builders`. I suggest starting the name with lower case letter.
altavir (Migrated from github.com) reviewed 2020-06-13 19:27:12 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmExpressions.kt
altavir (Migrated from github.com) commented 2020-06-13 19:27:11 +03:00
Copy Link

Add documentation since the interface is public.

Add documentation since the interface is public.
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:01:04 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/expressions/Builders.kt
@ -0,0 +1,23 @@
package scientifik.kmath.expressions
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:01:04 +03:00
Copy Link

This is a violation of Coding Conventions.
image

This is a violation of Coding Conventions. ![image](https://user-images.githubusercontent.com/38042667/84574662-1d07a880-add2-11ea-82bb-fb1cacc5be07.png)
altavir (Migrated from github.com) reviewed 2020-06-13 20:02:04 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmExpressions.kt
altavir (Migrated from github.com) commented 2020-06-13 20:02:04 +03:00
Copy Link

does it make sense to do invoke without operator? It seems that it should be renamed to compile. Those method need to be documented as well.

does it make sense to do invoke without `operator`? It seems that it should be renamed to `compile`. Those method need to be documented as well.
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:03:12 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmExpressions.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:03:12 +03:00
Copy Link

OK

OK
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:03:15 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmExpressions.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:03:14 +03:00
Copy Link

OK

OK
altavir (Migrated from github.com) reviewed 2020-06-13 20:05:35 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmExpressions.kt
altavir (Migrated from github.com) commented 2020-06-13 20:05:35 +03:00
Copy Link

It seems like compile is a better name.

It seems like `compile` is a better name.
altavir (Migrated from github.com) reviewed 2020-06-13 20:06:32 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmGenerationContext.kt
altavir (Migrated from github.com) commented 2020-06-13 20:06:32 +03:00
Copy Link

Could the constructor be made internal?

Could the constructor be made internal?
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:07:00 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmGenerationContext.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:06:59 +03:00
Copy Link

I'll check

I'll check
altavir (Migrated from github.com) reviewed 2020-06-13 20:07:15 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Builders.kt
altavir (Migrated from github.com) commented 2020-06-13 20:07:15 +03:00
Copy Link

rename to asmBuilders

rename to `asmBuilders`
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:08:44 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Builders.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:08:44 +03:00
Copy Link

Source file names have to be started with upper-case letter. https://kotlinlang.org/docs/reference/coding-conventions.html#source-file-names

I'll rename to AsmBuilders

Source file names have to be started with upper-case letter. https://kotlinlang.org/docs/reference/coding-conventions.html#source-file-names I'll rename to AsmBuilders
altavir (Migrated from github.com) reviewed 2020-06-13 20:10:17 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Builders.kt
altavir (Migrated from github.com) commented 2020-06-13 20:10:17 +03:00
Copy Link

This method should never be made public, the whole compilation chain: creation of the context, invokation of expression and generation should hidden from API in order to avoid leaking of unfinilized AsmGenerationContext.

This method should never be made public, the whole compilation chain: creation of the context, invokation of expression and generation should hidden from API in order to avoid leaking of unfinilized AsmGenerationContext.
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:10:36 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Builders.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:10:36 +03:00
Copy Link

OK

OK
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:11:34 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/AsmGenerationContext.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:11:34 +03:00
Copy Link

It can.

It can.
altavir (Migrated from github.com) reviewed 2020-06-13 20:12:51 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Optimization.kt
altavir (Migrated from github.com) commented 2020-06-13 20:12:51 +03:00
Copy Link

I suggest to bundle all misc methodsin one file or create internal directory for them to limit their visibility.

I suggest to bundle all misc methodsin one file or create `internal` directory for them to limit their visibility.
altavir (Migrated from github.com) reviewed 2020-06-13 20:18:17 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/expressions/Builders.kt
@ -0,0 +1,23 @@
package scientifik.kmath.expressions
altavir (Migrated from github.com) commented 2020-06-13 20:18:16 +03:00
Copy Link

Yes, but there are some variations of file naming accepted by the communitiy. And there is a style that is used accross other modules, so we need to keepr it uniform. I will write a doc file with local conventions.

Yes, but there are some variations of file naming accepted by the communitiy. And there is a style that is used accross other modules, so we need to keepr it uniform. I will write a doc file with local conventions.
CommanderTvis (Migrated from github.com) reviewed 2020-06-13 20:18:20 +03:00
kmath-ast/src/jvmMain/kotlin/scientifik/kmath/asm/Optimization.kt
CommanderTvis (Migrated from github.com) commented 2020-06-13 20:18:20 +03:00
Copy Link

OK

OK
altavir (Migrated from github.com) approved these changes 2020-06-13 21:06:16 +03:00
Sign in to join this conversation.
Reviewers
No reviewers
altavir
Labels
Clear labels   
bug

Something isn't working

  
dependencies

Pull requests that update a dependency file

  
discussion

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
feature

A new feature request

  
good first issue

The issue awaits its hero. Contributions are welcome

  
misc

Infrastructure tasks, cosmetic changes and miscellaneous tasks

  
performance

Performance optimization issue

  
question

Further information is requested

  
test

Add tests for some functionality

  
use case

A use case for library or feature

  
wontfix

This will not be worked on

No Label
bug
dependencies
discussion
documentation
duplicate
feature
good first issue
misc
performance
question
test
use case
wontfix
Milestone
Clear milestone
No items
No Milestone
0.1.x
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: kscience/kmath#94
No description provided.
Delete Branch "adv-expr"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?