In D52398#1244074, @aaronpuchert wrote:

No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two of you with so many review requests lately, and I hope it'll soon be over.

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.

In D52339#1242202, @erik.pilkington wrote:

From the last line in the paper, it seems that C++ compatibility is a goal of the paper (or at least a consideration). We should probably think about this if/when the final wording gets accepted though.

In D52339#1242118, @jfb wrote:

In D52339#1242103, @aaron.ballman wrote:

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

It's already been proposed to WG14 and is currently on the SD-3 list of features to consider for C2x. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2008.pdf. I know Clive and am happy to point him towards this patch (when it lands) as demonstration of industry desire for the feature, in case he needs to provide updated papers.

Wonderful! Does this match he proposed C2x semantics? Once voted in we'll want to change this from just an extension to also be a -std=c2x thing, better have them match now.

In D52339#1242086, @jfb wrote:

I think we should consider proposing this to the C committee. @aaron.ballman I can help Erik write the paper, would you be able to present it? Too tight for the upcoming meeting, but I'm guessing we have *plenty* of time for ++C17.

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!

LGTM!

LGTM!

Adding a reviewer who knows more about ObjC than I do.

LGTM!

LGTM!

LGTM, with a minor formatting nit.

LGTM, with a request for the documentation.

LGTM with the nits corrected (there are still some formatting concerns).

LGTM!

In D51880#1230221, @JonasToth wrote:

In D51880#1229513, @aaron.ballman wrote:

Missing tests and changes to Registry.cpp for dynamic matchers.

Also, do you want to add isInstantiationDependent() at the same time, given the relationship with the other two matchers?

Do you mean a matcher that does return Node.isValueDependent() || Node.isTypeDependent() or hasAncestor(expr(anyOf(isValueDependent(), isTypeDependent())))?

Missing tests and changes to Registry.cpp for dynamic matchers.

LGTM!

In D51192#1226341, @steveire wrote:

Thanks.

The arc tool already inserted Differential Revision: into my git commit, but that's not what I wonder about. I'm looking for something I can insert into my git commit so that the bug will automatically be closed when I commit (and so that the bug will get a link to this PR when the PR is created). Does such a thing exist here?

In D51192#1226326, @steveire wrote:

As far as I know, no existing clang-tidy checks are affected. I discovered this while implementing a custom check. See https://bugs.llvm.org/show_bug.cgi?id=38678

In D51192#1226312, @steveire wrote:

In D51192#1226282, @aaron.ballman wrote:

I'd probably pipe the diagnostic output to a temporary file that gets FileChecked with strict whitespace to see if the underlines from the diagnostic match the expected locations. We do this in a few Clang tests, like SemaCXX\struct-class-redecl.cpp or Misc\wrong-encoding.c.

Doesn't this require building-in a new check to clang-tidy which exists only for the purpose of the test? Otherwise how would a test similar to SemaCXX\struct-class-redecl.cpp work? What would be in the RUN line?

In D51192#1226257, @steveire wrote:

How? This is 'private' code. I don't think it's worth changing that or creating a test with a huge amount of infrastructure in order to test this indirectly.

Thank you for this! I have some cursory review comments, and possibly more later.

LGTM

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!

I think the fix generally looks good, but can you please add some test coverage for the change?

In D51333#1219938, @rsmith wrote:

Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the file that was intended, but we could also recover from the error by including the trimmed filename.

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).

I've commit in r340918. Thank you for the patch!

In D51061#1215917, @hugoeg wrote:

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive and I don't have commit access. Thank you for your time!

In D51132#1215916, @hugoeg wrote:

@aaron.ballman when you get the chance could you take another look at this and commit if it is ready? My internship ends rather soon and this is a tad time sensitive. Thank you for your time!

I'm not opposed to the changes here, but I am wondering what the benefits are to splitting this off into its own function?

Aside from the formatting and one small documentation nit, LGTM!

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?

LGTM!

Some minor nits that can be handled post-commit.

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.

LGTM, though you don't need review for fixing bots.

LGTM!

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!

LGTM!

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.

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.

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)?

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.

In D50110#1187828, @aaronpuchert wrote:

For now I think I'm done fixing things in the thread safety analysis, but if I see myself contributing more in the longer term, I will definitely try to obtain commit access.

In D49910#1187492, @mboehme wrote:

In D49910#1187455, @aaron.ballman wrote:

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I don't have any experience contributing to libc++, but I think this would make sense.

The check currently hard-codes various member functions of classes in the "std" namespace that do reinitializations; I'm not sure though if those can be removed after the attribute has been added to libc++. We'd would also presumably have to add the attribute to libstdc++ -- does it accept Clang-only attributes? And what is the story for people using clang-tidy with MSVC projects? (I have to admit I'm very hazy on how that works...)

In D50110#1187771, @aaronpuchert wrote:

Thanks for the review! Could you commit for me again?

LGTM!

Are you going to propose adding this attribute to libc++, or is this expected to only work with UDTs?

I am still okay with this, and agree that the ordering issues are a separate thing to tackle.