njames93 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:176
public:
- DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions,
- const ClangTidyOptions &Options)
- : GlobalOptions(GlobalOptions), DefaultOptions(Options) {}
+ DefaultOptionsProvider(ClangTidyGlobalOptions GlobalOptions,
+ ClangTidyOptions Options)
----------------
Quuxplusone wrote:
> If you're refactoring ctors anyway, perhaps add `explicit` to all of them?
> (The only reason to have a non-`explicit` ctor is if you want to enable
> callers to write `DefaultOptionsProvider d = {gopt, opt};` instead of `auto d
> = DefaultOptionsProvider(gopt, opt);`.) But this would be scope creep, of
> course.
`explicit`/`non-explicit` doesn't really affect much here. These constructors
are only used for constructing base classes or forwarding args to
`std::make_unique`.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:225
+ ClangTidyOptions &&OverrideOptions,
+ ConfigFileHandlers &&ConfigHandlers);
----------------
Quuxplusone wrote:
> I'd strongly recommend doing these two signatures in exactly the same way as
> you've done the others: pass by value and `std::move` out of it.
> Pass-by-rvalue-reference should be reserved for arcane stuff like
> hand-written move-constructors. You don't need pass-by-rvalue-reference here.
While I agree that rvalues in constructors is usually suspicious, in this case
it does make sense.
Firstly, FileOptionsBaseProviders constructors aren't exposed to the public
interface, so we don't really need to worry on that front.
Using r-values saves unneeded move constructor calls, which given that
ClangTidyGlobalOptions and ClangTidyOptions have non-trivial most constructors,
this is a slight win.
Is it still better to just pass by value though??
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92267/new/
https://reviews.llvm.org/D92267
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
