- User Since
- Mar 14 2013, 3:16 PM (288 w, 5 d)
Mon, Sep 24
Unlike checking const qualifiers on member functions, there are probably not many false positives here: if a function takes a non-const reference, it will in almost all cases modify the object that we passed it.
LGTM, but you should give @delesley a chance to weigh in before you commit.
Aside from the local const qualification stuff and some minor wordsmithing of the documentation, this LGTM.
Updated based on review feedback.
Sat, Sep 22
Fri, Sep 21
Whoops, that acceptance was accidental. Pretending I want changes just to make it clear in Phab. :-)
I really like this idea in general, thank you for proposing this patch!
Thu, Sep 20
Tue, Sep 18
Adding a reviewer who knows more about ObjC than I do.
Mon, Sep 17
LGTM, with a minor formatting nit.
LGTM, with a request for the documentation.
LGTM with the nits corrected (there are still some formatting concerns).
Tue, Sep 11
Mon, Sep 10
Missing tests and changes to Registry.cpp for dynamic matchers.
Thu, Sep 6
Thank you for this! I have some cursory review comments, and possibly more later.
Tue, Sep 4
Thank you for pointing out that the new form of type attributes aren't automatically opted in from this change. With that (and the two problematic attributes opted out), this LGTM.
I've commit in r341361, thank you for the patch!
Fri, Aug 31
I think the fix generally looks good, but can you please add some test coverage for the change?
Thu, Aug 30
LGTM with a commenting nit. Thank you for this, I like this exposition better!
I'd like to see some C++ tests that check reference behavior (and perhaps more esoteric pointerish non-pointer types like member function pointers).
Wed, Aug 29
I've commit in r340918. Thank you for the patch!
Tue, Aug 28
Mon, Aug 27
I'm not opposed to the changes here, but I am wondering what the benefits are to splitting this off into its own function?
Aug 24 2018
Aside from the formatting and one small documentation nit, LGTM!
I'm not certain how I feel about this as it seems to be removing functionality users may be relying on that I think is still technically correct. Can you describe more about why you think this should change?
Aug 23 2018
Aug 21 2018
Some minor nits that can be handled post-commit.
Aug 14 2018
A few more minor nits to be cleared up, but otherwise LGTM. You should wait for @rsmith to sign off before committing in case he has further comments, however.
Aug 13 2018
LGTM, though you don't need review for fixing bots.
Aug 12 2018
LGTM! An attribute really is the right way to go for this sort of thing, and it sounds like you've got a base of users looking for the functionality.
In general, I think this is the right way to go. I've added @rsmith to the reviewers because I'm curious what his thoughts are on this approach, though.
LGTM! Thank you for this fantastic work!
Thank you for the patch and great discussion! I've commit in r339516.
Alex has had plenty of time to respond, so I'm fine handling any concerns he has post-commit. Do you need me to commit this on your behalf?
Please be sure to regenerate the AST matcher documentation as well, as I'd expect to see some documentation changes from this.
LGTM, but please split out the unrelated changes into their own commit.
Aug 10 2018
Oops, I hit "Send" too soon. I was going to say that you should also keep an eye on D50526 because that may impact this patch (or vice versa if this one lands first).
Thank you for working on this -- the approach you've taken looks good, and I mostly have little nits here and there. I think this is a great refactoring overall though -- much needed!
Committed in r339456; if @delesley has concerns with the commit, they can be addressed post-commit. Thank you for the patch!
Thank you for the review and discussion -- I've commit in r339455.
Aug 6 2018
The functionality is looking good, aside from a few small nits remaining. However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter? Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?
Aug 5 2018
I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!
I think this is close. If @alexfh doesn't chime in on the open question in the next few days, I would say the check is ready to go in and we can address the open question in follow-up commits.
Aug 3 2018
Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?
Aug 2 2018
I am still okay with this, and agree that the ordering issues are a separate thing to tackle.