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.

Sign up for GitHub

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

Jump to bottom

C++20 Support #1235

Merged
merged 1 commit into from
Feb 15, 2024
Merged

C++20 Support #1235

merged 1 commit into from
Feb 15, 2024

Conversation

paulfd
Copy link
Member

@paulfd paulfd commented Feb 4, 2024

From @KKQ-KKQ

@paulfd
Copy link
Member Author

paulfd commented Feb 4, 2024

@KKQ-KKQ if you can check that this build with your toolchain thatd be great. I made a couple changes to reduce the number of ifdef and fix some other issues that popped up.

@paulfd paulfd marked this pull request as ready for review February 10, 2024 21:24
KKQ-KKQ
KKQ-KKQ reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@KKQ-KKQ KKQ-KKQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late review.
I commented some days ago but the comments remained "Pending".

I don't think there is always compatibility between std::string and std::u8string especially in file paths.

clients/sfizz_render.cpp Outdated
@@ -165,7 +166,7 @@ int main(int argc, char** argv)
ERROR_IF(!synth.loadSfzFile(sfzPath), "There was an error loading the SFZ file.");
LOG_INFO(synth.getNumRegions() << " regions in the SFZ.");

fmidi_smf_u midiFile { fmidi_smf_file_read(midiPath.u8string().c_str()) };
fmidi_smf_u midiFile { fmidi_smf_file_read(from_u8string(midiPath.u8string()).c_str()) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this works in Windows environment with non-ASCII characters?
I cannot confirm it because I am with Mac.
Windows uses ANSI, UTF-16, and UTF-8.
ANSI and UTF-8 are different.

If you don't want to support non-ASCII characters, it might be Ok.

Copy link
Contributor

@KKQ-KKQ KKQ-KKQ Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmidi_smf_file_read(const char *) uses ANSI, as far as I confirmed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm are you sure? From this it seems that the POSIX version would use fopen (which will be UTF-8 on linux afaik, not sure about macOS) and has some conversion from utf8 to utf16 to open on Windows. At least running this patch on windows for sfizz_render works properly for me.

Did you have any failing scenario in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I misunderstood. I was wrong.
I'm sorry.
It use UTF-8 even on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the code on my mac with the vs code and the wrong #if block was grayed, I misunderstood.
Sorry for confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change?
In my opinion, it is a little bit complex process.

src/sfizz/Synth.cpp Show resolved Hide resolved
@KKQ-KKQ
Copy link
Contributor

KKQ-KKQ commented Feb 13, 2024 •
edited
Loading

It might be better that we add functions like pathToString, stringToPath, pathToU8String and u8StringToPath.
On Windows, string means ANSI, u8String means UTF-8, wstring means UTF-16 as far as I know.
but std::u8string is sometimes hard to use, so we use std::string as a UTF-8 string container.
It might be better that we should name the string variables with a prefix/postfix 'u8'.

@paulfd
Copy link
Member Author

paulfd commented Feb 13, 2024

No worries. We can add some functions if needed, but after reading https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r2.html I felt their proposal was reasonable.

@paulfd
Copy link
Member Author

paulfd commented Feb 13, 2024

If this patch works for you and your toolchain, we can solve problems as they arise and possibly add functions at that time!

@KKQ-KKQ
Copy link
Contributor

KKQ-KKQ commented Feb 13, 2024

It is hard for me to apply this patch into my local mod without changes because I don't manage my mod with git.
I only checked if this branch works with sfizz-ui and actually it works.

@paulfd paulfd merged commit 698e27f into develop Feb 15, 2024
11 checks passed
@paulfd paulfd deleted the paulfd/import-kkq-cpp20 branch February 15, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKQ-KKQ KKQ-KKQ KKQ-KKQ left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulfd @KKQ-KKQ