search

blt4linux / blt4l

PAYDAY 2 SteamOS/Linux LUA loader.
Other
57 stars 14 forks source link

fs_delete_dir is broken, plus suggestions #42

Closed Ozymandias117 closed 7 years ago

Ozymandias117 commented 7 years ago

I went to pull up and noticed the addition of fs_delete_dir...

if (currentNode->d_name == "." || currentNode->d_name == "..") is comparing two c-style strings, so you're only comparing a memory address. Also shouldn't return after removing the first file on 188.

Not actually sure how that hasn't wiped anyone's hard drive yet... I just wiped mine while trying to figure out how it hasn't already happened... I would suggest this implementation, which seems less error prone than just fixing that function: https://stackoverflow.com/questions/3184445/how-to-clear-directory-contents-in-c-on-linux-basically-i-want-to-do-rm-rf/3184915#3184915

Can I also suggest we get rid of -fpermissive in the CMake script? We should probably really use -Wall -Werror and fix the bugs that the compiler already knows about...

Another thing I noticed on this branch is that SubHook is crashing whenever we go down to the "won't load" branch in blt_main, since we have statically initialized SubHooks, and the destructor in the subhook submodule doesn't verify the hook exists before dereferencing in the destructor.

Finally, why do we have the threadList in http.cc? Why not just create the threads and detach them? We're never rejoining, anyway...

Wanted to at least open a discussion before just making some of those changes.

RomanHargrave commented 7 years ago

I'm a bit busy with school at the moment, but you are welcome to open a PR to fix it. I'll see about getting around to it tomorrow.

Ozymandias117 commented 7 years ago

Pushed the changes for threads and delete_directory. Haven't tested the changes to zip.cc enough to add be able to start building with -Wall/-Wextra yet.

RomanHargrave commented 7 years ago

@Ozymandias117 Those two commits on lgtm at first glance, ran some updates.

WRT "the changes to zip.cc" could you perhaps keep them available in a feature branch so that I can look at them until you are ready to merge them? I'm going to go ahead and run the builds for binary dist.

RomanHargrave commented 7 years ago

Closing fixed.