Resolved Warnings and Improved Error Handling and fixed memory issue and updated project settings

[neel5682]

fixed memory issues by optimizing project settings.
Resolved compiler warnings to improve code quality.
Enhanced error handling for better stability.
Updated dependencies and JVM options for improved performance.

[szymon-rd]

There are a lot of files in this MR. Only changes to the source code should be in the diff.

[szymon-rd]

G1 is the default GC so no need to enable it

[szymon-rd]

I don't think it is a good idea. It's pretty similar to letting people write fors in a language without upper bound, and having default = 100, it can be quite confusing. If you would like to make it more ergonomic you could make a GSeq.reduce result only be a Value after limit is called on it. That would be quite challenging but a good Scala excercise.

[szymon-rd]

It's to be removed after addressing previous comment, but regardless, if we are to introduce warnings it's not a good idea to println. You usually want to use some logging API that can be configured/adjusted by library user, SLF4J is a good example.

[szymon-rd]

👍

[neel5682]

Greetings sir, Thank you for reviewing my pull request and providing valuable feedback. I’ve gone through your comments and here’s my plan to address them: 1. *Too many files in the PR* – I'll review the changes and ensure only relevant source code modifications are included. 2. *G1GC flag* – Since G1GC is the default garbage collector, I’ll remove the unnecessary flag. 3. *Default limit in GSeq.scala* – I understand the concern about implicit defaults being confusing. I’ll explore modifying GSeq.reduce so that it only returns a value after limit is explicitly set, as you suggested. 4. *Using println for warnings* – I’ll replace it with a proper logging mechanism like SLF4J for better configurability. I’ll implement these changes and update the PR shortly. Please let me know if you have any additional suggestions. Best regards, Neel

…

On Thu, 27 Feb 2025 at 23:20, Simon R ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In build.sbt <#8 (comment)>: > @@ -30,6 +30,9 @@ lazy val lwjglNatives = { val lwjglVersion = "3.3.3" val jomlVersion = "1.10.0" +javaOptions += "-Xmx8G" +javaOptions += "-XX:+UseG1GC" G1 is the default GC so no need to enable it ------------------------------ In src/main/scala/io/computenode/cyfra/dsl/GSeq.scala <#8 (comment)>: > @@ -119,12 +117,12 @@ object GSeq: val streamNextExpr = seq.source.next val seqExprs = seq.elemOps.map(_.fn) - val limitExpr = ConstInt32(seq.limit.getOrElse(throw new IllegalArgumentException("Reduce on infinite stream is not supported"))) + // Ensure a default limit to avoid infinite stream errors + val defaultLimit = 100 // Set a safe default limit I don't think it is a good idea. It's pretty similar to letting people write fors in a language without upper bound, and having default = 100, it can be quite confusing. If you would like to make it safer you could make a GSeq reduce result only be a Value after limit is introduced. That would be quite challanging but a good Scala excercise. ------------------------------ In src/main/scala/io/computenode/cyfra/dsl/GSeq.scala <#8 (comment)>: > @@ -119,12 +117,12 @@ object GSeq: val streamNextExpr = seq.source.next val seqExprs = seq.elemOps.map(_.fn) - val limitExpr = ConstInt32(seq.limit.getOrElse(throw new IllegalArgumentException("Reduce on infinite stream is not supported"))) + // Ensure a default limit to avoid infinite stream errors + val defaultLimit = 100 // Set a safe default limit + val limitExpr = ConstInt32(seq.limit.getOrElse { + println("⚠ Warning: No limit set, using default limit of " + defaultLimit) It's to be removed after addressing previous comment, but regardless, if we are to introduce warnings it's not a good idea to println. You usually want to use some logging API that can be configured/adjusted by library user, SLF4J is a good example. ------------------------------ In src/main/scala/io/computenode/cyfra/spirv/compilers/ExpressionCompiler.scala <#8 (comment)>: > @@ -60,6 +60,7 @@ private[cyfra] object ExpressionCompiler: case (from, _: ToUInt32[_]) if from.tag =:= Float32Tag.tag => Op.OpConvertFToU case (from, _: ToInt32[_]) if from.tag =:= UInt32Tag.tag => Op.OpBitcast case (from, _: ToUInt32[_]) if from.tag =:= Int32Tag.tag => Op.OpBitcast + case _ => throw new MatchError(s"Unexpected input: (${cexpr.fromTag}, $cexpr)") 👍 — Reply to this email directly, view it on GitHub <#8 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCJNAH7LAJYFBWG7PTPBAQD2R5F7TAVCNFSM6AAAAABXVFGBXCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNBYGYYDCOJRHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[neel5682]

Greetings sir, I have successfully resolved all the warnings and compilation issues in the Cyfra project. However, there is one issue I did not push yet because I wanted to clarify it with you first. The issue was related to the *Playground* object name causing a case sensitivity warning. I can rename it to resolve the warning completely, but I wanted to confirm with you before making any changes. Should I proceed with renaming *Playground* and then push the changes? Please let me know your preference. Best regards, Neel

…

On Thu, 27 Feb 2025 at 23:52, Neel ***@***.***> wrote: Greetings sir, Thank you for reviewing my pull request and providing valuable feedback. I’ve gone through your comments and here’s my plan to address them: 1. *Too many files in the PR* – I'll review the changes and ensure only relevant source code modifications are included. 2. *G1GC flag* – Since G1GC is the default garbage collector, I’ll remove the unnecessary flag. 3. *Default limit in GSeq.scala* – I understand the concern about implicit defaults being confusing. I’ll explore modifying GSeq.reduce so that it only returns a value after limit is explicitly set, as you suggested. 4. *Using println for warnings* – I’ll replace it with a proper logging mechanism like SLF4J for better configurability. I’ll implement these changes and update the PR shortly. Please let me know if you have any additional suggestions. Best regards, Neel On Thu, 27 Feb 2025 at 23:20, Simon R ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In build.sbt > <#8 (comment)>: > > > @@ -30,6 +30,9 @@ lazy val lwjglNatives = { > > val lwjglVersion = "3.3.3" > val jomlVersion = "1.10.0" > +javaOptions += "-Xmx8G" > +javaOptions += "-XX:+UseG1GC" > > G1 is the default GC so no need to enable it > ------------------------------ > > In src/main/scala/io/computenode/cyfra/dsl/GSeq.scala > <#8 (comment)>: > > > @@ -119,12 +117,12 @@ object GSeq: > val streamNextExpr = seq.source.next > val seqExprs = seq.elemOps.map(_.fn) > > - val limitExpr = ConstInt32(seq.limit.getOrElse(throw new IllegalArgumentException("Reduce on infinite stream is not supported"))) > + // Ensure a default limit to avoid infinite stream errors > + val defaultLimit = 100 // Set a safe default limit > > I don't think it is a good idea. It's pretty similar to letting people > write fors in a language without upper bound, and having default = 100, it > can be quite confusing. If you would like to make it safer you could make a > GSeq reduce result only be a Value after limit is introduced. That would > be quite challanging but a good Scala excercise. > ------------------------------ > > In src/main/scala/io/computenode/cyfra/dsl/GSeq.scala > <#8 (comment)>: > > > @@ -119,12 +117,12 @@ object GSeq: > val streamNextExpr = seq.source.next > val seqExprs = seq.elemOps.map(_.fn) > > - val limitExpr = ConstInt32(seq.limit.getOrElse(throw new IllegalArgumentException("Reduce on infinite stream is not supported"))) > + // Ensure a default limit to avoid infinite stream errors > + val defaultLimit = 100 // Set a safe default limit > + val limitExpr = ConstInt32(seq.limit.getOrElse { > + println("⚠ Warning: No limit set, using default limit of " + defaultLimit) > > It's to be removed after addressing previous comment, but regardless, if > we are to introduce warnings it's not a good idea to println. You > usually want to use some logging API that can be configured/adjusted by > library user, SLF4J is a good example. > ------------------------------ > > In > src/main/scala/io/computenode/cyfra/spirv/compilers/ExpressionCompiler.scala > <#8 (comment)>: > > > @@ -60,6 +60,7 @@ private[cyfra] object ExpressionCompiler: > case (from, _: ToUInt32[_]) if from.tag =:= Float32Tag.tag => Op.OpConvertFToU > case (from, _: ToInt32[_]) if from.tag =:= UInt32Tag.tag => Op.OpBitcast > case (from, _: ToUInt32[_]) if from.tag =:= Int32Tag.tag => Op.OpBitcast > + case _ => throw new MatchError(s"Unexpected input: (${cexpr.fromTag}, $cexpr)") > > 👍 > > — > Reply to this email directly, view it on GitHub > <#8 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/BCJNAH7LAJYFBWG7PTPBAQD2R5F7TAVCNFSM6AAAAABXVFGBXCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNBYGYYDCOJRHA> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[szymon-rd]

Hi, honestly I feel like this MR has a bit too much AI in it - especially not in the code but in the overall direction and replies. It also added few new printlns, and that's not a good idea for core parts of a library (as I pointed out with slf4j). AI really won't work well in this library.

[neel5682]

I appreciate your feedback! Honestly, I use AI mainly for error clarification and debugging assistance rather than for generating major code changes. As for the println statements, that’s part of my usual debugging habit, but I understand that they shouldn't be in core parts of the library. I’ll make sure to clean them up and be more mindful of this in the future. Best regards Neel

…

On Tue, 4 Mar 2025, 23:19 Simon R, ***@***.***> wrote: Hi, honestly I feel like this MR has a bit too much AI in it - especially not in the code but in the overall direction and replies. It also added few new printlns, and that's not a good idea for core parts of a library (as I pointed out with slf4j). AI really won't work well in this library. — Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCJNAHYSADCYJRWBZ3TSAWL2SXRS5AVCNFSM6AAAAABXVFGBXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJYGQ2TMNJTHE> . You are receiving this because you authored the thread.Message ID: ***@***.***> [image: szymon-rd]*szymon-rd* left a comment (ComputeNode/cyfra#8) <#8 (comment)> Hi, honestly I feel like this MR has a bit too much AI in it - especially not in the code but in the overall direction and replies. It also added few new printlns, and that's not a good idea for core parts of a library (as I pointed out with slf4j). AI really won't work well in this library. — Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCJNAHYSADCYJRWBZ3TSAWL2SXRS5AVCNFSM6AAAAABXVFGBXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJYGQ2TMNJTHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>