search

sahib / rmlint

Extremely fast tool to remove duplicates and other lint from your filesystem
http://rmlint.rtfd.org
GNU General Public License v3.0
1.91k stars 132 forks source link

Alt mains tidy up #483

Closed SeeSpotRun closed 3 years ago

SeeSpotRun commented 3 years ago

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.

cebtenzzre commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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.