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

QLockFile instead of QLockedFile #21557

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sakgoyal
Copy link

@sakgoyal sakgoyal commented Oct 9, 2024

This code seems weird to have as I believe it is based on qt4. I came across the built in qt version in the docs, but the interface is NOT the same.

Using the qt provided version would be better right?

or is the locking mode that necessary to the functionality?

you can also get the source here:
https://github.com/qt/qtbase/tree/dev/src/corelib/io

I could also just copy in the current qt version and add the locking modes myself. Let me know what is preferred.

This does seem to introduce breaking changes. I am not sure what effects this will have because I am not sure how to test the change

@glassez
Copy link
Member

glassez commented Oct 9, 2024

@sakgoyal
Why would you want to interfere with something that works without problems? Especially if you can't even test it:

I am not sure what effects this will have because I am not sure how to test the change

@xavier2k6
Copy link
Member

This does seem to introduce breaking changes.

Well, let's see how our CI behaves for an indication!

@sakgoyal
Copy link
Author

sakgoyal commented Oct 9, 2024

Why would you want to interfere with something that works without problems?

because I believe that code that qt provides (and updates regularly) is going to be more secure/battle tested than a custom implementation implementation from 2013 that does not get updated. (the legal notice from qt still says 2013)

Well, let's see how our CI behaves for an indication!

it compiles fine. and CI didnt complain either. but the logic has changed so I dont know what the effects of it are. (you no longer have the option to write-only or read-only lock)

@Chocobo1
Copy link
Member

it compiles fine. and CI didnt complain either. but the logic has changed so I dont know what the effects of it are. (you no longer have the option to write-only or read-only lock)

It would be insane to approve such changes when nobody knows whether it will break the app logic or not. Then what is the point of such PR? To be honest, I'm inclined to just reject it.

@glassez
Copy link
Member

glassez commented Oct 10, 2024

because I believe that code that qt provides (and updates regularly) is going to be more secure/battle tested than a custom implementation

It isn't "custom implementation". It was borrowed from Qt a long time ago. In fact, it was even used by Qt itself in QtCreator until recently. But now it looks like they are using QLockFile.

PS. We could learn from their experience and borrow current implementation of QtLocalPeer.

@sakgoyal
Copy link
Author

PS. We could learn from their experience and borrow current implementation of QtLocalPeer

where is it currently implemented? I could not find a reference to it anywhere.

@xavier2k6
Copy link
Member

@sakgoyal
Copy link
Author

sakgoyal commented Nov 2, 2024

@xavier2k6 I was asking about the implementation for QtLocalPeer, not QLockFile.

@xavier2k6
Copy link
Member

I was asking about the implementation for QtLocalPeer, not QLockFile

I know, I was only putting link there for quick reference....

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