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

Fixing code for MSVC #23

Merged
merged 7 commits into from
Jan 12, 2024
Merged

Fixing code for MSVC #23

merged 7 commits into from
Jan 12, 2024

Conversation

NotRequiem
Copy link
Collaborator

Compiler flags:

image

Compilation errors found in the previous version:

image
image

Fixed in the FIRST COMMIT

To see all the changes made in the first commit, you should click on "Ignore whitespaces", like this:
image

Link: NotRequiem@600d8b7?diff=unified&w=1

Compilation warnings found in the previous version:

Fixed some IntelliSense warnings about buffer overruns, variants not being initialized with VariantInit and 'dll could be 0'.

Fixed in the SECOND COMMIT

How did I fix

Normally in large source files, when fixing errors in a source file using Visual Studio, you may find no errors when you finish. But if you copy and paste the same source code with the same compiler flags in other project, using Visual Studio, more errors/warnings may be shown. Example of pasting the same code in another project:
image
image

So, if you constantly fix errors and warnings until you find no errors in other projects, then it will compile succesfully for everyone.

In the third commit, I fix every new warning/error that pops up whenever I try to paste the code in other projects with the same compiler and compiler flags

Link: NotRequiem@ccfd50e?diff=unified&w=0

Remember that most issues with compilation errors are because Unicode compatibility issues. Therefore, you need to enfornce "Use Unicode Character Set"

Using unicode character set:
image

Not using it:
image

Testing

The final code works for C++ 17:
image

The final code works for C++ 14:
image

The final code compiles correctly on other projects:
image

Final result:
image

Used const std::string& for vm_brand parameter in memoize function to avoid unnecessary string copies.

Updated the conditions in memoize to use bitwise AND (&) instead of logical AND (&&) for better efficiency.

Added comments to explain the purpose of the code.
@NotRequiem
Copy link
Collaborator Author

The compilation error in the build test is because the unicode character set that I have explained, "Remember that most issues with compilation errors are because Unicode compatibility issues. Therefore, you need to enfornce "Use Unicode Character Set"". Its just one problematic line thought.

@kernelwernel
Copy link
Owner

kernelwernel commented Jan 6, 2024

oh wow, that’s a massive PR! I was sleeping until I heard an email notification on my phone from github about this.

I’ll go back to sleep right now since my head is feeling fuzzy at the moment but I will review it whenever I wake up.

I genuinely appreciate the PRs you’ve made recently, it really means a lot :)

@kernelwernel
Copy link
Owner

hey, i made a PR on your fork here a few days ago. Just letting you know

@kernelwernel
Copy link
Owner

fixed the conflict in this PR.

@NotRequiem
Copy link
Collaborator Author

nice, I will review the code when I have time, everything seems ok tho and I'm agree with everything you mentioned, including the indentation part

@kernelwernel
Copy link
Owner

nice, I will review the code when I have time, everything seems ok tho and I'm agree with everything you mentioned, including the indentation part

take your time, there’s no rush :)
also, i do agree with the way you formatted preprocessor conditions, it looks better and clearer now

@NotRequiem
Copy link
Collaborator Author

Ignore the compatibility issue with WCHAR and char *:

image

image

image

Did you get the same IntelliSense errors?

@kernelwernel
Copy link
Owner

Ignore the compatibility issue with WCHAR and char *:

image

image

image

Did you get the same IntelliSense errors?

i have for the preprocessor thing but not for the other errors. I'm using /Wall /std:c++20 flags if that matters. Also, is it possible if you can merge my pull request if possible?

Fixed conflicts and made indentation changes
@NotRequiem
Copy link
Collaborator Author

Done, pull request merged.

@kernelwernel
Copy link
Owner

kernelwernel commented Jan 12, 2024

i'll try to get it fixed on my end, thanks for the effort btw! :)

If you want to contribute more then feel free to share new detection ideas, there's no stupid suggestion.

EDIT: fixed here, that was a mess up on my part

@kernelwernel kernelwernel merged commit dd65274 into kernelwernel:main Jan 12, 2024
0 of 3 checks passed
@NotRequiem
Copy link
Collaborator Author

perfect, I'll keep contributing to this project, I like it a lot

Thanks for your effors too, good job

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