Skip to content
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

Big-endian 'emojis.bin' decoding fix #100

Merged
merged 5 commits into from
Aug 14, 2022
Merged

Conversation

dragontamer8740
Copy link
Contributor

@dragontamer8740 dragontamer8740 commented Aug 13, 2022

See #97
Now synced up with master branch instead of attachments.

It finished compiling at last, and it does work fine on my PowerPC G4 (7447a) and amd64 (nehalem) machines both.

re: long compile times (from the other PR): yeah, it does take a while... but the powerbook is also just a dog performance-wise. It's much more bearable on my other hardware, especially because my other hardware has multiple execution cores.

@ouwou
Copy link
Member

ouwou commented Aug 13, 2022

looks good just clang-format it (and if it turns auto { to auto{ in line 93 just delete the -> auto i guess)

@dragontamer8740
Copy link
Contributor Author

dragontamer8740 commented Aug 14, 2022

What? And why?

Never used clang-format before, don't even have it installed, and I'm not sure what it's supposed to do or where I am supposed to run it.

I'm installing it, but I really don't know why I'm doing this or what the point is.

Also, line 93 of what? chatinput.cpp?

BTW, when I run it like clang-format < chatinput.cpp > new.cpp and then diff chatinput.cpp and new.cpp I get no differences.

@ouwou
Copy link
Member

ouwou commented Aug 14, 2022

its for formatting but if you dont want to deal with it i think i can add it to the pr myself? ive never done that before though

@dragontamer8740
Copy link
Contributor Author

dragontamer8740 commented Aug 14, 2022

See the update.

BTW, when I run it like clang-format < chatinput.cpp > new.cpp and then diff chatinput.cpp and new.cpp I get no differences.

$ clang-format --version
Debian clang-format version 14.0.6-2

Trying to add clang 15 to my system now just to see if that changes something for some reason. But I didn't touch line 93 so i don't know why that's something I need to be worrying about/messing with in this PR... shouldn't that be something separate if we're changing other code not related to the endian fixes?

Edit:

$ clang-format-15 < chatinput.cpp > a.cpp
$ diff chatinput.cpp a.cpp
$ clang-format-15 --version
Debian clang-format version 15.0.0-++20220625103012+3d37e785c77a-1~exp1

@dragontamer8740
Copy link
Contributor Author

Brain fart.

emojis.cpp. not chatinput.cpp.

OK, coming up.

@ouwou
Copy link
Member

ouwou commented Aug 14, 2022

the line 93 thing is because some versions of clang-format will screw up the trailing return type by removing the space even though it should be there

@dragontamer8740
Copy link
Contributor Author

dragontamer8740 commented Aug 14, 2022

That code already existed. So why wasn't this done already? Why is that change being introduced by my PR?

For the rest of it, the stuff I actually wrote, I understand.

And you want me to change

    auto get_text = [&]() -> auto{

to

    auto get_text = [&]() {

rather than

    auto get_text = [&]() -> auto {

Correct?

@ouwou
Copy link
Member

ouwou commented Aug 14, 2022

if clang-format doesnt do anything to the line then you can leave it be, and if it does modify it then you can just manually fix it so it remains unmodified. i just preemptively brought it up because clang-format might have incorrectly changed it.

@dragontamer8740
Copy link
Contributor Author

Which it did. Ok, so I will add the space back in.

@dragontamer8740
Copy link
Contributor Author

Done.

@ouwou ouwou merged commit fac9f1b into uowuo:master Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants