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

Contexts #65

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Contexts #65

wants to merge 5 commits into from

Conversation

S41L0R
Copy link

@S41L0R S41L0R commented Jul 26, 2023

This enables the use of PHYSFS with multiple different states at once, as discussed in #63.

@S41L0R S41L0R mentioned this pull request Jul 26, 2023
@sezero
Copy link
Collaborator

sezero commented Jul 30, 2023

Fixes C89 build failures in CI:

diff --git a/src/physfs.c b/src/physfs.c
index 998157c..958b784 100644
--- a/src/physfs.c
+++ b/src/physfs.c
@@ -3498,10 +3498,11 @@ static int doDeinitContext(Context *context) {
 int PHYSFS_initContext(PHYSFS_Context _context, const char *argv0)
 {
     Context *context = (Context*)_context;
+    PHYSFS_Context boundContext;
 
     BAIL_IF(context->initialized, PHYSFS_ERR_IS_INITIALIZED, 0);
 
-    PHYSFS_Context boundContext = PHYSFS_getBoundContext();
+    boundContext = PHYSFS_getBoundContext();
     PHYSFS_bindContext(_context);
 
     if (!initializeContextMutexes(context)) goto initFailed;
@@ -3554,15 +3555,19 @@ int PHYSFS_deinitContext(PHYSFS_Context _context)
 
 int PHYSFS_bindContext(PHYSFS_Context context)
 {
+    ThreadContextMapNode *newNode;
+    ThreadContextMapNode *currentNode;
+    ThreadContextMapNode *lastNode;
+    void *threadID;
+
     __PHYSFS_platformGrabMutex(threadContextMapLock);
 
-    void *threadID = __PHYSFS_platformGetThreadID();
-    
+    threadID = __PHYSFS_platformGetThreadID();
     if (!context)
         context = defaultContext;
 
-    ThreadContextMapNode *currentNode = threadContextMap;
-    ThreadContextMapNode *lastNode = threadContextMap;
+    currentNode = threadContextMap;
+    lastNode = threadContextMap;
     if (currentNode)
     {
         do
@@ -3579,7 +3584,7 @@ int PHYSFS_bindContext(PHYSFS_Context context)
     } /* if */
     else
     {
-        ThreadContextMapNode *newNode = allocator.Malloc(sizeof(ThreadContextMapNode));
+        newNode = allocator.Malloc(sizeof(ThreadContextMapNode));
         GOTO_IF(!newNode, PHYSFS_ERR_OUT_OF_MEMORY, bindFailed);
         newNode->context = context;
         newNode->threadID = threadID;
@@ -3588,7 +3593,7 @@ int PHYSFS_bindContext(PHYSFS_Context context)
         goto bindSuccess;
     } /* else */
 
-    ThreadContextMapNode *newNode = allocator.Malloc(sizeof(ThreadContextMapNode));
+    newNode = allocator.Malloc(sizeof(ThreadContextMapNode));
     GOTO_IF(!newNode, PHYSFS_ERR_OUT_OF_MEMORY, bindFailed);
     newNode->context = context;
     newNode->threadID = threadID;
@@ -3607,11 +3612,14 @@ bindFailed:
 
 PHYSFS_Context PHYSFS_getBoundContext()
 {
+    ThreadContextMapNode *currentNode;
+    void *threadID;
+
     __PHYSFS_platformGrabMutex(threadContextMapLock);
 
-    void *threadID = __PHYSFS_platformGetThreadID();
+    threadID = __PHYSFS_platformGetThreadID();
+    currentNode = threadContextMap;
 
-    ThreadContextMapNode *currentNode = threadContextMap;
     while (currentNode)
     {
         if (currentNode->threadID == threadID)

@S41L0R
Copy link
Author

S41L0R commented Jul 31, 2023

Thanks so much! What a dumb mistake coming from C++.

@S41L0R
Copy link
Author

S41L0R commented Apr 17, 2024

Yo! This isn't a plea to merge it or anything, (obviously that's up to you), I just wanted to give a little update by saying that this fork has been stable in my project thus far. (As it should be, it's a pretty simple addition, despite the large diff count.) Hopefully that inspires some confidence (: I know stability was probably a concern with this one, because of how big of a change it is.

I also know it deviates from the implementation planned for V4. That is, this does not include passing the instance/context as the first parameter to functions, and instead uses per-thread context-switching. I remember I had originally implemented something like what was planned for V4, but it ended up resulting in a lot of actual code needing to be rewritten. This context-switching approach meant that I didn't really get the opportunity to introduce bugs in the same way, as it's a simple replacement of references. (You can check the diff to see what I mean.)

Anyway, I write all this to inspire some confidence. I know you've learned not to trust external contributors with big stuff, but I really don't think this one is all that bad. When you start work on V4, I hope you fiddle around with it and potentially merge it to your working branch.

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

2 participants