search

emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.91k stars 3.32k forks source link

Refactor lookupPath to avoid recursion. NFC #23017

Closed hoodmane closed 14 hours ago

hoodmane commented 1 week ago

This removes the recursion from lookupPath and in my opinion makes the control flow much more explicit and comprehensible.

hoodmane commented 1 week ago

Well it's going to conflict with #22998, I suppose you can pick which one to merge first.

sbc100 commented 1 week ago

Can you confirm those codesize changes are all due to this change? i.e. can you run the rebase on main and confirm you don't see any changes?

hoodmane commented 1 week ago

There are a lot of similar changes when I run it on main too.

sbc100 commented 1 week ago

There are a lot of similar changes when I run it on main too.

Maybe try with emsdk install tot (or otherwise make sure you have the latest llvm and binaryen). (Also try ./emcc --clear-cache)

hoodmane commented 1 week ago

Even after ./emsdk install tot, and clearing cache and getting the most up to date main branch I get a bunch of changes from the rebaseline.

sbc100 commented 1 week ago

Even after ./emsdk install tot, and clearing cache and getting the most up to date main branch I get a bunch of changes from the rebaseline.

Hmm.. that is strange/annoying. Maybe just revert those files then, and I can do a rebaseline after this lands.

sbc100 commented 1 week ago

OK, I just pushed another rebaseline commit, perhaps this one will match what you are generated on main now?

hoodmane commented 6 days ago

Nope, even if I check out your rebaseline commit and do a clean install of tot from emsdk I get a bunch of changes in code size by one to ten bytes.

hoodmane commented 1 day ago

I think this PR fixes the case when you symlink a directory to itself symlink(".", "dir") and then try to glob("**") it. The previous lookupPath doesn't seem to detect this loop.

hoodmane commented 23 hours ago

@sbc100 can this be merged?