[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-21 Thread Srividya Sundaram via Phabricator via cfe-commits
srividya-sundaram abandoned this revision. srividya-sundaram added a comment. I did spend a good amount of time trying to figure out how to abandon this review and gave up eventually. Even after your comment, I had to search a bit for the "Add Action.." drop down. Clearly, I'm UI challenged.

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-21 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Herald added a subscriber: sunshaoce. In D157129#4571112 , @srividya-sundaram wrote: > Based on your comments, and after discussing with @tahonermann, I am > abandoning this patch. Btw, it helps reviewers if you

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-08 Thread Srividya Sundaram via Phabricator via cfe-commits
srividya-sundaram added a comment. In D157129#4564766 , @steakhal wrote: > I went over the patch and I found only a single debatable case for taking by > reference. > To clarify why I would prefer taking by value: value semantics is good for > local

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-08 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann requested changes to this revision. tahonermann added a comment. I agree with the analysis done by @steakhal. The static analyzer being used (which I'm not allowed to name) is being too noisy in these cases; it complains about every case where an object of class type is copied

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-07 Thread Balázs Benics via Phabricator via cfe-commits
steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I went over the patch and I found only a single debatable case for taking by reference. To clarify why I would prefer taking by value: value semantics is good for local

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-04 Thread Srividya Sundaram via Phabricator via cfe-commits
srividya-sundaram updated this revision to Diff 547350. srividya-sundaram added a comment. Added 'const' type qualifiers per review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157129/new/ https://reviews.llvm.org/D157129 Files:

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-04 Thread Balázs Benics via Phabricator via cfe-commits
steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I can only vouch for the SttaticAnalyzer portion. Those parts makes sense to me. However, I've seen cases where it doesn't resonate. As a rule of thumb, if we are "copying" only

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

2023-08-04 Thread Srividya Sundaram via Phabricator via cfe-commits
srividya-sundaram created this revision. Herald added subscribers: luke, steakhal, frasercrmck, martong, luismarques, apazos, sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, johnrusso, rbar,