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

! checking if each match found is either a file or folder appears to … #1850

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

Conversation

mihaisebea
Copy link

@mihaisebea mihaisebea commented Apr 3, 2022

…be expensive on large projects (at least on windows)

! for a 5k files project GetAttributesW takes around 20% of the time

What does this PR do?

Thanks for the contribution! Please provide a concise description of the problem this request solves.

How does this PR change Premake's behavior?

Are there any breaking changes? Will any existing behavior change?

Anything else we should know?

Add any other context about your changes here.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

…be expensive on large projects (at least on windows)

! for a 5k files project GetAttributesW takes around 20% of the time
Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

LGTM.

@samsinsane
Copy link
Member

Noticed the tests fail on non-Windows platforms, it fails on os.remove() when attempting to remove a directory. os.remove() is supposed to fail, but it succeeds now. I can't really figure out what's different other than os.matchisfile will check the dirent->d_type if it has it before falling back to calling os.isfile.

! fix indeting
@mihaisebea
Copy link
Author

Noticed the tests fail on non-Windows platforms, it fails on os.remove() when attempting to remove a directory. os.remove() is supposed to fail, but it succeeds now. I can't really figure out what's different other than os.matchisfile will check the dirent->d_type if it has it before falling back to calling os.isfile.

I noticed and it has been puzzling me for a while :(
I need to get my hands on a mac or linux so this will take a bit.

table.insert(results, matchpath)
elseif matchType == "folder" and not os.matchisfile(m) then
table.insert(results, matchpath)
else -- keep previous behaviour
Copy link
Member

Choose a reason for hiding this comment

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

I just realised it's this here, it needs to check that matchType == nil or something, otherwise all folders when matching files are picked up, and all files when matching folders are picked up.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition the comment is not meaningful outside of the pull request.

else
table.insert(results, matchpath)
if matchType == "file" and os.matchisfile(m) then
Copy link
Contributor

Choose a reason for hiding this comment

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

typo? for below too

Suggested change
if matchType == "file" and os.matchisfile(m) then
if matchType == "file" and os.matchisfile(matchpath) then

Copy link
Member

Choose a reason for hiding this comment

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

os.matchisfile takes a match context, which is m here, so this isn't a typo.

Copy link
Member

Choose a reason for hiding this comment

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

That's not how not (A and B) works, either A or B can be false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, indeed, misread.

@nickclark2016
Copy link
Member

Just wanted to poke this and see if there's a status on it since there are changes requested.

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.

5 participants