Review takes too long to make forward progress; abandoning.

Review process takes too long to make forward progress; abandoning.

Review takes too long to make forward progress; abandoning.

In D56323#1350998, @JonasToth wrote:

@LegalizeAdulthood your stuck on the commit right things right? If you want I can commit for you (maybe even later) as you said you are limited on time.

To avoid passing lots of state to/from extracted functions, extract a class instead and use member variables for most of the state

I managed to do some limited testing on the original source file from the bug report in lldb and verified that the correct fix was applied there. I also tried a few other files and verified no false positives.

Uh... I'm developing on Windows, which doesn't have a solution for generating compile_commands.json, so I can't easily test this on the entire clang codebase. I will test on a source file based on the original bug report.

No really, update from review comments :)

Update from review comments

In D56323#1347450, @JonasToth wrote:

Did you check if the changes actually fix the problematic behaviour? Did you run it over any project and did the code-transformation break the code, false positives, ...?

Otherwise LGTM

Update from review comments

Update from review comments

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

Extract Function try_run

No, really, fix typo in id

Fix typo in id

Fix typo in id

clang-format

Other than a few minor nits, ship it.

In D28667#650586, @leanil wrote:

In D28667#650585, @LegalizeAdulthood wrote:

In D28667#649626, @LegalizeAdulthood wrote:

Don't do this without introducing an option to turn it off.

To clarify my reasoning:
You imposing your subjective idea of goodness on the code, i.e. style. Other people will have different ideas about what they want from the code. So, to quote the X Window System Maxim, provide tools not rules. Allow people to say whether or not they want the raw string literal in this case.

I agree. Which mode do you think should be the default?

In D28667#649626, @LegalizeAdulthood wrote:

Don't do this without introducing an option to turn it off.

Don't do this without introducing an option to turn it off.

That we've had to fix this twice now tells me that our collective memory is forgetful :).

There are some APIs where a series of strings are passed as a single char * with NUL separators between the strings and the last string is indicated by two NUL characters in a row. For instance, see the MSDN documentation for the lpDependencies argument to CreateService. Such strings literals in the code look like:

In D17482#390237, @alexfh wrote:

My main concern is that this change makes the already complicated and unobvious testing mechanism [...]

As it stands currently, you can't commit a header with CHECK-MESSAGES and CHECK-FIXES lines and have them verified.

In D18509#388071, @hintonda wrote:

With this change, won't you need to update 2 different repos: clang and extra? Is it possible to update both with a single Phabricator patch?

I'm sorry to be such a nag and I know you're busy, but....

Can you summarize the improvements in docs/ReleaseNotes.rst, please?

Can you summarize the improvements in docs/ReleaseNotes.rst please?

Can you summarize this check in docs/ReleaseNotes.rst in the clang-tidy section, please?

Maybe you need to rebase first? I haven't used arc.

Please summarize the improvements in docs/ReleaseNotes.rst in the clang tidy section.

Please summarize this check in docs/ReleaseNotes.rst in the clang-tidy section.

Please summarize this check in docs/ReleaseNotes.rst.

You can submit the release notes changes in a new patch.

Can you add a synopsis of this new check to docs/ReleaseNotes.rst please?

Can you add something to the docs/ReleaseNotes.rst for this new check, please?

Squeak

Remove brace initializers from test code.

Update release notes

Looks like I forgot to remove brace initializers from the test files. I will fix that.

test/clang-tidy/misc-unused-parameters.cpp currently does a hack to verify headers by committing the manually fixed header and diffing it:

In D17482#372677, @alexfh wrote:

Can you [...] say how many times this feature is going to be used, if it gets added to the testing script?

In D17482#372677, @alexfh wrote:

Sorry for leaving this without attention for a while.

I'm somewhat concerned about this change. It's adding a certain level of complexity, but doesn't cover some less trivial cases like handling of multiple headers.

Update from comments.
Update documentation to reflect changes in the implementation.

Update from review comments.

In D18191#377686, @sdowney wrote:

In D18191#376471, @LegalizeAdulthood wrote:

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

The usual pattern for that is to have the definition use const, and the declaration not, since it's an implementation detail if the passed parameter is modified. And, unfortunately, I'm stuck with one compiler that mangles those two differently, causing link errors.

Having void foo(const int i) in a declaration makes me suspicious, since the const is meaningless.

I could make this an option? If the option is set, emit 'const type' if the Decl hasBody?

Sort check names in list

LGTM

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

Another week without reviews...

I do not have commit access, so I will need someone to commit this for me.

I need to update from review comments and upload a new diff.

Discarding due to inaction by reviewers.

Discarding due to inaction by reviewers.

Discarding due to inaction by reviewers.

Discarding due to inaction by reviewers.

Squeak. This has been waiting on review for over two weeks....

Squeak

In D17484#358617, @alexfh wrote:

Thank you for this check! Mostly looks good, but there are a number of style nits.

The most important question to this check is that the standard doesn't guarantee that the C++ headers declare all the same functions in the global namespace (at least, this is how I understand the section of the standard you quoted).

Other than a few nits, LGTM.

make check-all passes in release

I do not have commit access, so someone else will need to commit this. Let me know if it needs rebasing.