cjdb added inline comments.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst:21
+
+ If set to `true`, this check will limit itself to the `builtinType()`
types. Default is `false`.
----------------
jmarrec wrote:
> cjdb wrote:
> > jmarrec wrote:
> > > Quuxplusone wrote:
> > > > D107873 is related.
> > > >
> > > > I'd like to see some tests/discussion around large types, e.g.
> > > > ```
> > > > struct Widget { int a[1000]; };
> > > > void f(const Widget&);
> > > > ```
> > > > (I think D107873 at least makes a conscious attempt to get the "would
> > > > it be passed in registers?" check right.)
> > > >
> > > > I'd like to see some tests/discussion around generic code, e.g.
> > > > ```
> > > > template<class T>
> > > > struct Max {
> > > > static T max(const T& a, const T& b);
> > > > };
> > > > int x = Max<int>::max(1, 2); // passes `int` by const reference, but
> > > > this is fine
> > > > ```
> > > Should I close this one in favor of https://reviews.llvm.org/D107873?
> > Up to you. There's always a chance that D107873 doesn't get approved. What
> > I find most interesting is that we were independently working on this at
> > the same time :-)
> @cjdb I guess it's uncanny that we would both decide to do it at the same
> time. A bit unfortunate too probably, I could have saved a few hours :) but I
> learned something about clang-tidy so I don't mind at all!
>
> Yours (D107873) is definitely better documented, you handle to opposite case
> (I only deal with const-ref -> value, not value -> const ret) and I just
> borrowed your implementation for MaxSize / ignoring shared_ptr
>
> As far as I can tell from a cursory look, there are a couple of things I'm
> doing that you aren't:
>
> * I am actually using Options, I don't think you are yet, even for MaxSize. I
> see from your documentation file that you plan to add a few
> * The RestrictToBuiltInTypes I find an interesting option too
> * I am removing the const as well in addition to the `&`. I guess you are
> just relying on using it in conjunction with
> `readability-avoid-const-params-in-decls` but I think that makes it harder to
> use (you can't just do -checks=-*,<yourcheck> you have to remember to add the
> readability-avoid-const-params-in-decls too...).
>
> Perhaps we could discuss adding the above to your PR and consolidate over
> there to make a nice, feature-complete check?
Sounds good.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107900/new/
https://reviews.llvm.org/D107900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
