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

Re-work is_source_file #9

Closed
wants to merge 1 commit into from

Conversation

yaaama
Copy link
Contributor

@yaaama yaaama commented Dec 20, 2024

Complete re-work of the is_source_file function. Changes include:

  • Using a lookup table for the extensions that are supported.
  • No need use macros to search for the extensions.

Benefits:

  • Code produces less assembly.
  • Looks nicer.
  • We can more easily add file extensions to support.

@yaaama yaaama changed the title refactor(dir.c): Re-implement 'is_source_file' Re-work is_source_file Dec 20, 2024
@agvxov
Copy link
Owner

agvxov commented Dec 24, 2024

The code only produces less assembly because you are calling into libc. Which almost certainly means the function is slower too. Now, this is the kind of performance difference that's not even worth talking about anyways, I'm only bringing it up because you mentioned assembly.

The 'looks nicer' is a bit subjective here. Pulling out the data (suffix list) is generally cleaner, but its actually a code segment that's not really going to change anyways; and the : delimiterring is strange in this context.
This is somewhat besides the point, however the commenting is a little excessive.

I have actually considered refactoring is_source_file before, but I have come to the conclusion that its not that bad at all.

I feel like that by merging this I would be swapping one ok implementation to another ok implementation.

@yaaama
Copy link
Contributor Author

yaaama commented Dec 29, 2024

The code only produces less assembly because you are calling into libc. Which almost certainly means the function is slower too. Now, this is the kind of performance difference that's not even worth talking about anyways, I'm only bringing it up because you mentioned assembly.

The 'looks nicer' is a bit subjective here. Pulling out the data (suffix list) is generally cleaner, but its actually a code segment that's not really going to change anyways; and the : delimiterring is strange in this context. This is somewhat besides the point, however the commenting is a little excessive.

I have actually considered refactoring is_source_file before, but I have come to the conclusion that its not that bad at all.

I feel like that by merging this I would be swapping one ok implementation to another ok implementation.

Good evening David,

your points are very much valid and correct. I will close this PR, thanks for looking over it and giving the helpful feedback!

@yaaama yaaama closed this Dec 29, 2024
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.

2 participants