LG. Thank you for the fix!

LG with a nit.

Still LG with one comment.

LG

LG with a nit.

LG

LG

LG with a nit.

In D34440#843192, @vladimir.plyashkun wrote:

@alexfh
Thanks for the response!
Yes, we re-implemented logic and now use JSON compilation database to pass compiler options to Clang-Tidy.
Anyway, i think in general it's useful to support @response_files, see my comment: https://reviews.llvm.org/D34440#813411

In D34440#825924, @vladimir.plyashkun wrote:

Many build systems normally generate response files on-fly in some circumstances (e.g. if command line is longer than some platform-imposed limit). So IMO response files should be a perfect citizen here.

friendly ping

In D35941#838633, @rsmith wrote:

In D35941#823524, @alexfh wrote:

IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway

That's not strictly true; the variable can be accessed in unevaluated operands, and in code doing so, a -Wshadow warning might (theoretically) be useful:

void f(SomeComplexType val) {
  struct A {
    decltype(val) &ref;
    void g(int val) {
      decltype(val) *p = &ref;
    }
  } a = {val};
}

Could you rebase the patch? It doesn't apply cleanly.

LG with a few nits.

LG

A few late comments.

A few more nits.

Thank you for the fix!
Looks good!

I'm not sure this approach is the best to deal with the boilerplate, but it seems like an improvement compared to repeating code. See a few comments inline.

Added redirections from the old check documents.

Looks good with one nit.

LG with a couple of nits.

In D36051#825428, @xazax.hun wrote:

Maybe instead of a separate list, having this information like yes/no column in a table in the original is more user-friendly.

LG with a couple of comments.

LG with one comment.

In D35941#823020, @rnk wrote:

I'm not really sure this is a bug. The point of -Wshadow is to warn on valid but possibly confusing code. Shadowing a variable is perfectly legal, but people think it's confusing, so we implemented this warning. Are we concerned that people might get confused between the different local variables here?

Could you run the check over LLVM+Clang code and post a summary of results here?

LG. Do you need someone to commit the patch for you?

In D35787#819007, @alexfh wrote:

Should we look at whether the size is statically known?

Should we look at whether the size is statically known?

A few more nits.

LG. Thank you for the fix!

LG. Thanks!

Committed as r308726.

LG. I'll commit the patch for you

In D24892#733016, @alexfh wrote:

In D24892#732243, @malcolm.parsons wrote:

In D24892#732217, @alexfh wrote:

In D24892#723536, @malcolm.parsons wrote:

Is there any way for multiple checks to share an option?

There's OptionsView::getLocalOrGlobal. See how StrictMode option is read in ArgumentCommentCheck, for example.

ArgumentCommentCheck uses getLocalOrGlobal, but InefficientStringConcatenationCheck and SuspiciousEnumUsageCheck don't.
6 checks have an IncludeStyle option that isn't shared.
4 checks share a HeaderFileExtensions option, but with different defaults.

The right thing would be to fix these. I might get around to this, if nobody does this before me.

The top of stack trace is:

clang::CXXRecordDecl::isTriviallyCopyable()
(unknown)
clang::tidy::bugprone::(anonymous namespace)::internal::matcher_isNotTriviallyCopyableMatcher::matches()

FYI, bugprone-undefined-memory-manipulation crashes on some of our code. I guess, this happens with dependent types, but I don't have an isolated repro yet.

In our project we want to do something like include src/.*.h but exclude src/some-thirdparty/.*.h.

I wonder whether anyone uses file patterns that need anything from regular expressions beyond | and .*. If not, globs (as used in -checks=) would be a better solution.

In D35372#810457, @alexfh wrote:

I have deja vu ;)

Should we make a single check for all CLOEXEC and friends with a single configurable list of (function name, flag name) pairs?

See the comment on D35372.

See the comment on D35372.

See the comment on D35372.

See the comment on D35372.

See the comment on D35372.

See the comment on D35372.

See the comment on D35372.

I have deja vu ;)

Please regenerate the patch with full context (e.g. as described in llvm.org/docs/Phabricator.html).

In D34440#809522, @klimek wrote:

In D34440#809325, @vladimir.plyashkun wrote:

Even if i'll change content of arguments.rsp to
-std=c++11 -Ipath/to/include -Ipath/to/include2 -DMACRO ....
and will try to call clang-tidy process in this way:
clang-tidy -checks=* main.cpp -export-fixes=... -- @arguments.rsp
it also has no effect, because all compiler options will be ignored (i thinks it's because that stripPositionalArgs() function deletes @arguments.rsp parameter as unused input).

Ah, ok: this is definitely the way it should work (adding the response file after the --). I think this is the use case we should fix - probably by fixing stripPositionalArgs?

In D34440#808156, @vladimir.plyashkun wrote:

To discuss:
...
By this moment, we unable to use CompilationDatabase.json from CLion side which is widely used in Clang-Tidy and in other common tools.

Looks good with a single comment.

LG. Thanks!

LG

LG. Thank you for the fix!

LG