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

Alt mains tidy up #483

Merged
merged 24 commits into from
Mar 27, 2021
Merged

Alt mains tidy up #483

merged 24 commits into from
Mar 27, 2021

Conversation

SeeSpotRun
Copy link
Collaborator

Addresses #458

Alternate mains --hash, --is-reflink and --dedupe all have stand-alone option parsing. The --[hash|is-reflink|dedupe] must be first arg.

Alternate main --gui --shredder does no option parsing but instead passes all args to the python script.

Note: rmlint --equal still uses main option parsing but that is probable appropriate.

SeeSpotRun added a commit that referenced this pull request Mar 27, 2021
@SeeSpotRun SeeSpotRun merged commit 807417f into sahib:develop Mar 27, 2021
@cebtenzzre
Copy link
Contributor

I have a feeling rm_sys_stat and rm_sys_lstat should have stayed inline because both are only one line long after preprocessing. If that's not small enough to be worth inlining then what is?

@SeeSpotRun
Copy link
Collaborator Author

SeeSpotRun commented Mar 27, 2021

I have a feeling rm_sys_stat and rm_sys_lstat should have stayed inline because both are only one line long after preprocessing. If that's not small enough to be worth inlining then what is?

You're probably right. I de-inlines the two procedures that called rm_log_perror as a lazy way to avoid a recursive header inclusion error. At the same time I noticed the comment:

"We should not use functions that are I/O bound as inline functions." ( https://www.tutorialspoint.com/when-to-use-inline-function-and-when-not-to-use-it-in-c-cplusplus )

It's probably a bit of a stretch to classify stat and lstat as I/O bound so maybe I over-reacted.

Thoughts?

@cebtenzzre
Copy link
Contributor

@SeeSpotRun They are likely I/O bound in the sense that the function call overhead is negligible compared to the cost of calling stat. But the inline version would produce simpler machine code that might be easier for the compiler to optimize, and would avoid the extra frame in the call stack at runtime.

For single line wrapper functions, I think it's good practice to make them inline or macros regardless of what they do.

@SeeSpotRun
Copy link
Collaborator Author

I was unsure of the reason for the recommendation "We should not use functions that are I/O bound as inline functions" and feared that they might make things worse. On reading further it seems the reason is that they don't give much speed improvement, rather than that they make things worse.

For single line wrapper functions, I think it's good practice to make them inline or macros regardless of what they do.

Ok, will revert those.

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