drahnr / cargo-spellcheck

Checks all your documentation for spelling and grammar mistakes with hunspell and a nlprule based checker for grammar
Apache License 2.0
314 stars 32 forks source link

Inline format args, optimize perf #314

Closed nyurik closed 8 months ago

nyurik commented 8 months ago

What does this PR accomplish?

Most of the time you do not need to use an & when calling various format_args! methods - it automatically does that already, plus it causes about 5% perf degradation (compiler cannot optimize most of them due to some limitations discussed at rustlang).

I tried to cleanup a bit, plus a few tiny spacing fixes. BTW, any reason not to remove log:: prefix? I.e. add a use log at the top?

📜 Checklist

drahnr commented 8 months ago

Hey @nyurik , thank you for your PR!

Most of the time you do not need to use an & when calling various format_args! methods - it automatically does that already, plus it causes about 5% perf degradation (compiler cannot optimize most of them due to some limitations discussed at rustlang).

Could you provide a link to the discussion? Note that most of the statements were written before the name based referencing as part of the format string was stabilized.

I tried to cleanup a bit, plus a few tiny spacing fixes. BTW, any reason not to remove log:: prefix? I.e. add a use log at the top?

I'd prefer to keep the prefix for logging. It's mostly habit for avoiding any ambiguities, which are less relevant in this project so far.

📜 Checklist

* [ ]  Works on the `./demo` sub directory

* [ ]  Test coverage is excellent and passes

* [ ]  Documentation is thorough

These should be ticked if you asserted the statements hold.

nyurik commented 8 months ago

Hey @nyurik , thank you for your PR!

Most of the time you do not need to use an & when calling various format_args! methods - it automatically does that already, plus it causes about 5% perf degradation (compiler cannot optimize most of them due to some limitations discussed at rustlang).

Could you provide a link to the discussion? Note that most of the statements were written before the name based referencing as part of the format string was stabilized.

See https://github.com/rust-lang/rust/issues/112156

📜 Checklist

* [ ]  Works on the `./demo` sub directory

* [ ]  Test coverage is excellent and passes

* [ ]  Documentation is thorough

These should be ticked if you asserted the statements hold.

I would love to tick it, but i cannot test it because clang fails to compile (which is weird - i am able to compile the compiler, and I'm on the latest Linux Mint, with clang installed - are there any dependencies you had to install to compile?)

drahnr commented 8 months ago

clang

To triage I need clang version and error output and env vars

drahnr commented 8 months ago

Could you force push after a rebase and git rid of the merge commit, ghs non-configurable buttons drive me nuts at times.

nyurik commented 8 months ago

Could you force push after a rebase and git rid of the merge commit, ghs non-configurable buttons drive me nuts at times.

done