cienicera / Koji

The First Onchain Generative Music Library. Deployed on Starknet
9 stars 11 forks source link

Upgrade cairo version to 2.6.3 #39

Closed AryanGodara closed 8 months ago

AryanGodara commented 8 months ago

Pull Request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: Resolves #35

What is the new behavior?

-

-

Does this introduce a breaking change?

Other information

AryanGodara commented 8 months ago

I'm curious, why add the '_' character before msg names like SetTempo to _SetTempo? Is that a style thing or something new to cairo 2.6?

It's because in the match case, SetTempo was used, but it's not used. So, it was a compiler warning

Message::SET_TEMPO(_SetTempo) => { eventlist.append(*currentevent); },

We can also write

Message::SET_TEMPO(_) => { eventlsit.append(...); },

but I thought to preserve the correct variable name, in case we use it in future

AryanGodara commented 8 months ago

In the TG you said you were getting some warnings like this: Some(varName) => { ...varName not used here... } // So compiler said to use _varName if it's not being used Thanks for the explanation in the previous comment... now that you changed those, are these warnings gone?

Yes, all warnings and errors are gone now :D

caseywescott commented 8 months ago

This looks good to me! I'll merge shortly! I do get the following unused variables in cubit but everything runs well and doesn't affect Koji:

Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f64/math/ops.cairo:209:10 let (div, rem) = core::integer::u64_safe_divmod(b.mag, u64_as_non_zero(ONE)); ^*^

warn: Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f128/math/ops.cairo:229:10 let (div_u128, rem_u128) = _split_unsigned(b); ^**^

AryanGodara commented 8 months ago

This looks good to me! I'll merge shortly! I do get the following unused variables in cubit but everything runs well and doesn't affect Koji:

Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f64/math/ops.cairo:209:10 let (div, rem) = core::integer::u64_safe_divmod(b.mag, u64_as_non_zero(ONE)); ^*^

warn: Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f128/math/ops.cairo:229:10 let (div_u128, rem_u128) = _split_unsigned(b); ^**^

Oh, I'm sorry. I think I missed those 😅 Is it okay if I quickly open a new PR to fix these as well? (Not part of the ODHack, just a quick fix :D )

caseywescott commented 8 months ago

Sure you can! I already pushed the last round of commits so you might need to make a new branch...

On Tue, Mar 26, 2024 at 9:20 PM Aryan Godara @.***> wrote:

This looks good to me! I'll merge shortly! I do get the following unused variables in cubit but everything runs well and doesn't affect Koji:

Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f64/math/ops.cairo:209:10 let (div, rem) = core::integer::u64_safe_divmod(b.mag, u64_as_non_zero(ONE)); ^*^

warn: Unused variable. Consider ignoring by prefixing with _. --> /Users/caseywescott/Library/Caches/com.swmansion.scarb/registry/git/checkouts/cubit-k9rijvovof89o/6275608/src/f128/math/ops.cairo:229:10 let (div_u128, rem_u128) = _split_unsigned(b); ^**^

Oh, I'm sorry. I think I missed those 😅 Is it okay if I quickly open a new PR to fix these as well? (Not part of the ODHack, just a quick fix :D )

— Reply to this email directly, view it on GitHub https://github.com/cienicera/Koji/pull/39#issuecomment-2020039914, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWPRSD7LMBBDSOEQEAJGI3Y2FDVDAVCNFSM6AAAAABE5S6ZGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRQGAZTSOJRGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

AryanGodara commented 8 months ago

@caseywescott It seems to be a warning in some library code, which isn't part of our repo. So, maybe we don't need to fix this? Since, it can't be pushed to git anyways 😅

caseywescott commented 8 months ago

Sounds good, everything is functioning properly so all good! Thanks for giving this a second look!!!

On Wed, Mar 27, 2024, 12:16 AM Aryan Godara @.***> wrote:

@caseywescott https://github.com/caseywescott It seems to be a warning in some library code, which isn't part of our repo. So, maybe we don't need to fix this? Since, it can't be pushed to git anyways 😅

— Reply to this email directly, view it on GitHub https://github.com/cienicera/Koji/pull/39#issuecomment-2020404780, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWPRSBC2IGXYRHU7R2K6GTY2FYKRAVCNFSM6AAAAABE5S6ZGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRQGQYDINZYGA . You are receiving this because you were mentioned.Message ID: @.***>