Page MenuHomePhabricator

ccotter (Chris Cotter)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 18 2022, 9:40 PM (13 w, 1 d)

Recent Activity

Sat, Mar 18

ccotter updated the diff for D141892: Implement modernize-use-constraints.

rebase

Sat, Mar 18, 5:17 PM · Restricted Project, Restricted Project

Sun, Mar 12

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Thanks!

Sun, Mar 12, 2:05 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Yes, I'm ready for this to be committed. I don't have commit access, but if anyone could commit this on my behalf with Chris Cotter <ccotter14@bloomberg.net> that'd be great!

Sun, Mar 12, 9:44 AM · Restricted Project, Restricted Project

Thu, Mar 2

ccotter added inline comments to D145138: [clang-tidy] Implement FixIts for C arrays.
Thu, Mar 2, 7:47 PM · Restricted Project, Restricted Project
ccotter updated the diff for D145138: [clang-tidy] Implement FixIts for C arrays.
  • Add option doc, remove auto
Thu, Mar 2, 7:46 PM · Restricted Project, Restricted Project

Wed, Mar 1

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref.

Wed, Mar 1, 7:22 PM · Restricted Project, Restricted Project
ccotter updated the diff for D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
  • Add another test case
Wed, Mar 1, 7:07 PM · Restricted Project, Restricted Project
ccotter updated the diff for D145138: [clang-tidy] Implement FixIts for C arrays.
  • Add more tests for whitespace
Wed, Mar 1, 6:57 PM · Restricted Project, Restricted Project
ccotter added inline comments to D145138: [clang-tidy] Implement FixIts for C arrays.
Wed, Mar 1, 6:47 PM · Restricted Project, Restricted Project
ccotter updated the summary of D145138: [clang-tidy] Implement FixIts for C arrays.
Wed, Mar 1, 6:44 PM · Restricted Project, Restricted Project
ccotter retitled D145138: [clang-tidy] Implement FixIts for C arrays from Implement FixIts for C arrays to [clang-tidy] Implement FixIts for C arrays.
Wed, Mar 1, 6:41 PM · Restricted Project, Restricted Project
ccotter requested review of D145138: [clang-tidy] Implement FixIts for C arrays.
Wed, Mar 1, 6:41 PM · Restricted Project, Restricted Project

Tue, Feb 28

ccotter added a comment to D140794: [ASTMatcher] Add coroutineBodyStmt matcher.

Thanks both! Assuming this passes build, would one of you mind committing this for me? Chris Cotter <ccotter14@bloomberg.net>

Tue, Feb 28, 7:31 PM · Restricted Project, Restricted Project

Mon, Feb 27

ccotter updated the diff for D140794: [ASTMatcher] Add coroutineBodyStmt matcher.

fix bad 'arc diff'

Mon, Feb 27, 6:22 PM · Restricted Project, Restricted Project
ccotter updated the diff for D140794: [ASTMatcher] Add coroutineBodyStmt matcher.
  • update release note, rebase
Mon, Feb 27, 6:21 PM · Restricted Project, Restricted Project
ccotter added a comment to D143877: [NFC] [clang] Forward forwarding reference.

Thanks!

Mon, Feb 27, 5:57 PM · Restricted Project, Restricted Project
ccotter added a comment to D143877: [NFC] [clang] Forward forwarding reference.

I don't have commit access - could you use Author: Chris Cotter <ccotter14@bloomberg.net>? Thanks!

Mon, Feb 27, 5:06 PM · Restricted Project, Restricted Project

Sun, Feb 26

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Thanks for the name suggestion

Sun, Feb 26, 6:52 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • More clean option name
Sun, Feb 26, 6:50 PM · Restricted Project, Restricted Project
ccotter added inline comments to D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
Sun, Feb 26, 6:43 PM · Restricted Project, Restricted Project
ccotter updated the diff for D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
  • Add comment regarding std::begin/end
Sun, Feb 26, 6:39 PM · Restricted Project, Restricted Project
ccotter added a comment to D140794: [ASTMatcher] Add coroutineBodyStmt matcher.

Bump - anyone else have any thoughts here?

Sun, Feb 26, 6:29 PM · Restricted Project, Restricted Project
ccotter updated the diff for D143971: [clang-tidy] Flag more buggy string constructor cases.
  • Add more cases to swapped params
Sun, Feb 26, 6:28 PM · Restricted Project, Restricted Project
ccotter added a comment to D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.

I'm not sure about the false positive case. Range based for loops reduce to calls to ADL lookup begin/end (or member function calls to begin/end). I believe the same "false positive" argument (whatever that may be) would apply in an equivalent manner to the member call case.

Sun, Feb 26, 6:27 PM · Restricted Project, Restricted Project
ccotter added inline comments to D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
Sun, Feb 26, 6:19 PM · Restricted Project, Restricted Project
ccotter updated the diff for D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
  • Add negative test case
  • Fix logic to only consider std:: or ADL calls
Sun, Feb 26, 6:15 PM · Restricted Project, Restricted Project
ccotter updated the diff for D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
  • fix bad merge in ReleaseNotes
Sun, Feb 26, 10:07 AM · Restricted Project, Restricted Project
ccotter added inline comments to D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
Sun, Feb 26, 10:02 AM · Restricted Project, Restricted Project
ccotter updated the diff for D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.
  • Address feedback
Sun, Feb 26, 10:02 AM · Restricted Project, Restricted Project

Sat, Feb 25

ccotter added a comment to D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert.

Friendly ping.

Sat, Feb 25, 1:39 PM · Restricted Project, Restricted Project
ccotter added a comment to D143877: [NFC] [clang] Forward forwarding reference.

@dblaikie thoughts?

Sat, Feb 25, 1:39 PM · Restricted Project, Restricted Project

Thu, Feb 23

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

> Imaginate that such trivial type could be for example 200KB in size

Thu, Feb 23, 12:19 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Trivial types should not be passed by rvalue reference, but by value per the diagram under http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing. I feel like adding an option to opt-out of this is missing the point of this check, and rvalue references of trivial type should just be fixed rather than adding an option to permit them.

Thu, Feb 23, 5:59 AM · Restricted Project, Restricted Project

Wed, Feb 22

ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Wed, Feb 22, 5:22 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Wed, Feb 22, 5:21 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • More matcher simplification
Wed, Feb 22, 5:21 PM · Restricted Project, Restricted Project

Tue, Feb 21

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Ok, all feedback should be addressed (thanks for the comment to consolidate into a single matcher) with appropriate options to relax the enforcement of the guideline.

Tue, Feb 21, 9:29 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Include non-deduced template types
Tue, Feb 21, 9:27 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Tue, Feb 21, 8:43 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Finish combining into a single matcher
  • Rename option and add docs
Tue, Feb 21, 8:37 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Tue, Feb 21, 7:02 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Use bool
  • Combine into a single matcher
Tue, Feb 21, 7:02 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Tue, Feb 21, 6:30 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Simplified matchers

Tue, Feb 21, 6:24 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Simplify matcher
Tue, Feb 21, 6:23 PM · Restricted Project, Restricted Project

Mon, Feb 20

ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Mon, Feb 20, 9:29 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Mon, Feb 20, 8:54 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
> If you add std::move you will get compilation error, if you add std::forward, everything will work fine. Simply Value& and && will evaluate to Value&, no rvalue reference here.
Mon, Feb 20, 8:48 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Ignore params of template type
  • Add option for unnamed params
Mon, Feb 20, 8:41 PM · Restricted Project, Restricted Project
ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Mon, Feb 20, 9:02 AM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

I was split on handling the case where the parameter that was never moved from was not named. For this guideline enforcement to flag all unmoved rvalue reference parameters, code that std::moves into an argument to a virtual method call could be especially confusing or dangerous with regards to whether the object was truly "moved" (move constructor/assignment invoked within the method call). E.g.,

Mon, Feb 20, 8:57 AM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};
Mon, Feb 20, 8:50 AM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Remove duplicated matchers
Mon, Feb 20, 8:38 AM · Restricted Project, Restricted Project

Sun, Feb 19

ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Add back -fno-delayed-template-parsing to test
Sun, Feb 19, 10:50 PM · Restricted Project, Restricted Project

Feb 17 2023

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

@PiotrZSL I added an option to allow disabling the strict behavior. It should address many of your examples (e.g., moving subobjects) . Let me know if you have more feedback.

Feb 17 2023, 5:06 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.

rebase again

Feb 17 2023, 5:04 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Rebased as well on top of the latest release notes

Feb 17 2023, 5:02 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Add back -fno-delayed-template-parsing to test
Feb 17 2023, 4:56 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Add StrictMode option, fix const&& bug
Feb 17 2023, 4:55 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

@PiotrZSL The last forward example you have should match my does_move_auto_rvalue_ref_param test, which is not flagged by the tool. Let me know if you have any other forward cases (preferred as full self contained examples) that the tool incorrectly flags, as getting forward / universal references cases are important for this tool.

Feb 17 2023, 4:54 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

Some of the examples you mentioned are indeed not compliant with guideline F.18. I have a test for at least one of the examples you gave (moves_deref_optional matches your first example). The guideline is fairly strict IMO, and I asked about this in https://github.com/isocpp/CppCoreGuidelines/issues/2026, but the response was that the guideline will remain with the requirement that the whole object needs to be moved. This would imply that the fix for cases like moves_deref_optional would be to change the parameter to be a value rather than rvalue reference of the type (and move the individual parts as desired). Example X&& obj as argument + for(auto& v : obj) { something(std::move(v.x)); } also violates F.18 since the whole object is not moved (just a subobject, although I'm a little confused about the loop also).

Feb 17 2023, 8:57 AM · Restricted Project, Restricted Project

Feb 16 2023

ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

poke - anyone mind reviewing this?

Feb 16 2023, 8:26 PM · Restricted Project, Restricted Project

Feb 13 2023

ccotter retitled D143971: [clang-tidy] Flag more buggy string constructor cases from [clang-tidy] Flag code with both string constructor arguments implicitly casted to [clang-tidy] Flag more buggy string constructor cases.
Feb 13 2023, 7:59 PM · Restricted Project, Restricted Project
ccotter added inline comments to D143971: [clang-tidy] Flag more buggy string constructor cases.
Feb 13 2023, 7:57 PM · Restricted Project, Restricted Project
ccotter updated the diff for D143971: [clang-tidy] Flag more buggy string constructor cases.
  • Add more cases to swapped params
Feb 13 2023, 7:55 PM · Restricted Project, Restricted Project
ccotter added inline comments to D143971: [clang-tidy] Flag more buggy string constructor cases.
Feb 13 2023, 6:29 PM · Restricted Project, Restricted Project
ccotter retitled D143971: [clang-tidy] Flag more buggy string constructor cases from Flag code with both string constructor arguments implicitly casted to [clang-tidy] Flag code with both string constructor arguments implicitly casted.
Feb 13 2023, 6:27 PM · Restricted Project, Restricted Project
ccotter requested review of D143971: [clang-tidy] Flag more buggy string constructor cases.
Feb 13 2023, 6:27 PM · Restricted Project, Restricted Project
ccotter updated the diff for D143873: [NFC] [llvm] Forward forwarding references.
  • Fix BitVector
Feb 13 2023, 4:33 PM · Restricted Project, Restricted Project
ccotter updated the diff for D143877: [NFC] [clang] Forward forwarding reference.
  • Add test
Feb 13 2023, 4:27 PM · Restricted Project, Restricted Project
ccotter added a comment to D143877: [NFC] [clang] Forward forwarding reference.

Realistically, I'm not sure if this change is all that valuable aside from abiding by the CppCoreGuidelines from the readability and consistency standpoint. The only impact this change would have that I can think of is allowing an object with an && (or other combination) qualified operator() that would be picked up by this change, which doesn't seem likely of a use case for this function. I'll try to add a small test that covers this and leave it to the maintainers whether this is a worthy change :)

Feb 13 2023, 3:41 PM · Restricted Project, Restricted Project
ccotter added a comment to D143468: [CMake] Remove custom ccache CMake logic.

Another benefit is (and I might have been using the options incorrectly, which furthers the pointd made earlier I think) is that the recursive cmake invocations did not end up forwarding the LLVM_CCACHE_BUILD value, so only the top level build would be using ccache. The *COMPILER_LAUNCHER variable seems to get passed down correctly, although I'm sure if that's explicit or a happy coincidence.

Feb 13 2023, 11:10 AM · Restricted Project, Restricted Project, Restricted Project

Feb 12 2023

ccotter retitled D143873: [NFC] [llvm] Forward forwarding references from [NFC] [llvm] Forward forwarding reference pack to [NFC] [llvm] Forward forwarding references.
Feb 12 2023, 9:33 PM · Restricted Project, Restricted Project
ccotter updated the diff for D143873: [NFC] [llvm] Forward forwarding references.
  • Add more
Feb 12 2023, 9:32 PM · Restricted Project, Restricted Project
ccotter requested review of D143877: [NFC] [clang] Forward forwarding reference.
Feb 12 2023, 9:30 PM · Restricted Project, Restricted Project
ccotter requested review of D143873: [NFC] [llvm] Forward forwarding references.
Feb 12 2023, 8:19 PM · Restricted Project, Restricted Project
ccotter added a comment to D137302: [clang-tidy] Add modernize-type-traits check.

Looks good to me - looking forward to this check!

Feb 12 2023, 12:55 PM · Restricted Project, Restricted Project
ccotter added a comment to D143468: [CMake] Remove custom ccache CMake logic.

Looks good to me including the cmake logic. I've been building with the launcher environment variables since I posted the Discourse post. I can't really speak to the build bot side, which doesn't look to have been updated based on the most recently build of this diff.

Feb 12 2023, 12:52 PM · Restricted Project, Restricted Project, Restricted Project
ccotter added inline comments to D143851: [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class..
Feb 12 2023, 12:48 PM · Restricted Project, Restricted Project

Feb 5 2023

ccotter added inline comments to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.
Feb 5 2023, 9:09 PM · Restricted Project, Restricted Project
ccotter added a comment to D139122: Generalize clang-tidy modernize-pass-by-value.

https://reviews.llvm.org/D137205 looks to be similar in part, as it applies a FixIt to a any last use of an object if the use a copy, including parameters and local objects.

Feb 5 2023, 9:08 PM · Restricted Project, Restricted Project
ccotter added inline comments to D139122: Generalize clang-tidy modernize-pass-by-value.
Feb 5 2023, 8:40 PM · Restricted Project, Restricted Project

Feb 4 2023

ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Feb 4 2023, 2:44 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Add parameter name to diagnostic
Feb 4 2023, 2:34 PM · Restricted Project, Restricted Project

Feb 2 2023

ccotter added a comment to D141892: Implement modernize-use-constraints.

This is more or less ready for review (not planning on making any further changes; there are more features to be added, but I was thinking of handling those in follow up changesets). I know it's a relatively large review, but let me know if anyone can take a first pass. Thanks!

Feb 2 2023, 8:08 PM · Restricted Project, Restricted Project

Feb 1 2023

ccotter added a comment to D142825: [llvm][NFC] Use move instead of copy.

Yes, locally I get

Feb 1 2023, 8:54 AM · Restricted Project, Restricted Project
ccotter added a reviewer for D142825: [llvm][NFC] Use move instead of copy: Michael137.
Feb 1 2023, 8:12 AM · Restricted Project, Restricted Project
ccotter added a comment to D142824: [lldb][NFC] Use move instead of copy.

This should do: Chris Cotter <ccotter14@bloomberg.net>

Feb 1 2023, 7:30 AM · Restricted Project, Restricted Project

Jan 31 2023

ccotter added a comment to D142824: [lldb][NFC] Use move instead of copy.

Thanks! I don't have push access, so if you're good with this, could you please land this for me?

Jan 31 2023, 5:44 PM · Restricted Project, Restricted Project
ccotter added a reviewer for D142824: [lldb][NFC] Use move instead of copy: aprantl.
Jan 31 2023, 5:28 PM · Restricted Project, Restricted Project

Jan 30 2023

ccotter added inline comments to D141133: [clang-tidy] Implement CppCoreGuideline F.54.
Jan 30 2023, 6:06 AM · Restricted Project, Restricted Project

Jan 29 2023

ccotter added a comment to D141133: [clang-tidy] Implement CppCoreGuideline F.54.

Gentle poke for any last reviews?

Jan 29 2023, 8:33 PM · Restricted Project, Restricted Project

Jan 28 2023

ccotter updated the diff for D141892: Implement modernize-use-constraints.

Rebase + Simplify match logic

Jan 28 2023, 6:35 PM · Restricted Project, Restricted Project
ccotter requested review of D142825: [llvm][NFC] Use move instead of copy.
Jan 28 2023, 5:57 PM · Restricted Project, Restricted Project
ccotter requested review of D142824: [lldb][NFC] Use move instead of copy.
Jan 28 2023, 5:54 PM · Restricted Project, Restricted Project
ccotter added a comment to D141569: [clang-tidy] Implement CppCoreGuideline F.18.

In https://github.com/isocpp/CppCoreGuidelines/issues/2026, it looks like the core guideline will not permit exceptions for code that accepts an rvalue ref parameter, and the function body moves members of the parameter. So, my moves_member_of_parameter test case would be flagged. In theory, we could add a tool option to allow someone to live more "dangerously" and have the tool accept code like moves_member_of_parameter, with the caveat that the tool (as I have written it so far) would not be smart enough to detect that all static paths move every data member of the object. In any case, I'll update my test and the tool to align exactly with the guideline.

Jan 28 2023, 9:05 AM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.

rebase

Jan 28 2023, 7:41 AM · Restricted Project, Restricted Project

Jan 27 2023

ccotter updated the diff for D141133: [clang-tidy] Implement CppCoreGuideline F.54.

rebase

Jan 27 2023, 10:23 PM · Restricted Project, Restricted Project

Jan 24 2023

ccotter added inline comments to D141569: [clang-tidy] Implement CppCoreGuideline F.18.
Jan 24 2023, 9:30 PM · Restricted Project, Restricted Project
ccotter updated the diff for D141569: [clang-tidy] Implement CppCoreGuideline F.18.
  • Use multiple runs on the same test file
Jan 24 2023, 9:29 PM · Restricted Project, Restricted Project