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

Migrate to bindbc #612

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Migrate to bindbc #612

merged 1 commit into from
Jun 2, 2022

Conversation

Superbelko
Copy link
Contributor

BindBC is a newer loader mechanism that replaces Derelict, as Derelict is now basically discontinued I've migrated dlangui to use bindbc instead.

Note: provided .dll/.so needs to be updated as well, otherwise it will fail to load as the provided FreeType and SDL2 libs are too old

@Superbelko
Copy link
Contributor Author

I've updated previous work and merged with master, tested on Windows and Ubuntu 22.04 (both X11 and SDL2 backends).
Works with current provided binaries just fine.

Should be good to go now, please merge.

@@ -11,7 +11,7 @@
"targetName": "dminer",
"targetType": "executable",

"sourceFiles-windows": ["$PACKAGE_DIR/src/win_app.def"],
"sourceFiles-windows-x86": ["$PACKAGE_DIR/src/win_app.def"],
Copy link

Choose a reason for hiding this comment

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

What about windows x64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x86 arch uses old DMD linker (do not confuse it with x86_mscoff), Microsoft linker doesn't understands this and emits warning, but instead of completely removing this let's put it where it belongs to.

Copy link

Choose a reason for hiding this comment

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

I see

@@ -136,7 +135,8 @@ class MyOpenglWidget : VerticalLayout {
return;
}
bool canUseOldApi = !!glLightfv;
bool canUseNewApi = !glSupport.legacyMode;
//bool canUseNewApi = !glSupport.legacyMode;
Copy link

Choose a reason for hiding this comment

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

Commented code should be removed or uncommented

@@ -24,6 +24,7 @@ import dlangui.core.linestream;
import dlangui.core.streams;
import std.algorithm;
import std.conv : to;
import std.uni;
Copy link

Choose a reason for hiding this comment

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

Should this modules be imported at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, can be made static though.

static alias isDigit = std.uni.isNumber;

src\dlangui\core\editable.d(1211,28): Deprecation: package std.uni is not accessible here, perhaps add 'static import std.uni;'

// NOTE: this has crappy complexity as it was just updated as is
// it also does not checks if the symbol was actually loaded
auto errMsg = cast(string) info.message[0 .. info.message.strlen];
bool found = names
Copy link

Choose a reason for hiding this comment

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

I guess this code is wrong because it finds errMsg in names instead of names in errMsg. Also using array to count items is a bad practice.

Suggested change
bool found = names
bool found = any!(s=>errMsg1.canFind(s))(names);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's why there is a note, it is definitely ugly but this code is basically just run once on startup, it doesn't do much harm, so it can be improved later

Copy link

@drug007 drug007 May 23, 2022

Choose a reason for hiding this comment

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

It's not just ugly, it's wrong. Considering you add this code it should be fixed in this PR, not later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree it sucks, but the overall logic still valid, the problem is that it searches for substring in array of strings - it doesn't matter whether it searches name in errors or errors for name, in any case it is MxN task, but indeed as it was updated just as is there also another loop which makes it even worse.

It will throw exception if some symbols were unable to be loaded, another option would be to simply log an error and let it crash on null pointer call at some stage later which I think is far more frustrating to both programmer and a user.

I'll see what I can do to improve it.

Copy link

Choose a reason for hiding this comment

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

Well, as I understand it does matter whether it searches name in errors or errors for name - definitely you will always fail if you try to find the whole error message in a set of function names just because the error message is longer than some function name and contain other words than a function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Indeed that was my overlook, looking at phobos unit tests I misinterpret that canFind will pick up substring as well, but what's really needed is indexOf from std.string

// NOTE: this has crappy complexity as it was just updated as is
// it also does not checks if the symbol was actually loaded
auto errMsg = cast(string) info.message[0 .. info.message.strlen];
bool found = names
Copy link

Choose a reason for hiding this comment

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

I guess this code is wrong because it finds errMsg in names instead of names in errMsg. Also using array to count items is a bad practice.

Suggested change
bool found = names
bool found = any!(s=>errMsg1.canFind(s))(names);

// NOTE: this has crappy complexity as it was just updated as is
// it also does not checks if the symbol was actually loaded
auto errMsg = cast(string) info.message[0 .. info.message.strlen];
bool found = names
Copy link

Choose a reason for hiding this comment

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

I guess this code is wrong because it finds errMsg in names instead of names in errMsg. Also using array to count items is a bad practice.

Suggested change
bool found = names
bool found = any!(s=>errMsg1.canFind(s))(names);

@GrimMaple
Copy link
Collaborator

Can you please rebase your changes on master. instead of merging with it? I'm afraid we will have a lot of garbage in git history if it's not rebased.

@Superbelko
Copy link
Contributor Author

I've fixed error checking issue and squished the commit, should be ok now.

@GrimMaple GrimMaple merged commit d92f668 into buggins:master Jun 2, 2022
@GrimMaple
Copy link
Collaborator

Thank you for your contribution!

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.

None yet

3 participants