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

lcesave.hexpat: use Virtual Filesystem + ZLib support + auto endian detection #309

Merged
merged 4 commits into from
Nov 17, 2024

Conversation

DexrnZacAttack
Copy link
Contributor

No description provided.

import std.mem;
import std.core;
import hex.dec;
#ifdef __IMHEX__
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the __IMHEX__ define come from?

Copy link
Contributor

@paxcut paxcut Nov 2, 2024

Choose a reason for hiding this comment

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

It is defined internally before pattern is evaluated. All patterns have it. In the source code you will find it in content_registry.cpp line 612

 runtime.addDefine("__IMHEX__");
 runtime.addDefine("__IMHEX_VERSION__", ImHexApi::System::getImHexVersion());

Patterns shouldn't test for it, that's done internally in includes\hex\impl\imhex_check.pat

Copy link
Contributor

Choose a reason for hiding this comment

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

DexrnZacAttack, why are you testing for this define?

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't my place to reply since you asked the author directly, but note that the commit that adds the test for the define has "ifdefs to fix actions moment " as comment, so I suspect that it has to do with unit tests which wouldn't define the IMHEX variable because tests don't use the runtime. Also it would make little sense to warn user during a test run which is supposed to run automatically.

Copy link
Contributor

@C3pa C3pa Nov 2, 2024

Choose a reason for hiding this comment

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

Thanks, Paxcut.

Copy link
Contributor Author

@DexrnZacAttack DexrnZacAttack Nov 11, 2024

Choose a reason for hiding this comment

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

Thanks, Paxcut.

Just saw this thread, yes this is the case, as the actions stuff was failing without that check.

@WerWolv WerWolv merged commit bc5a55a into WerWolv:master Nov 17, 2024
2 checks passed
@WerWolv
Copy link
Owner

WerWolv commented Nov 17, 2024

Thank you!

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.

4 participants