In D68694#1711035, @aaron.ballman wrote:

LGTM aside from a small nit.

In D68694#1706204, @JonasToth wrote:

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

The files reside in clang-tools-extra/docs/ReleaseNotes.rst and clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst.

You can check other files for reference, e.g. readability-magic-numbers.rst.
In the Release Notes you can add one entry at the end of the clang-tidy section.

In D68694#1705668, @JonasToth wrote:

The new switch needs documentation as well, and maybe even a note in the release notes (as it is publicly discussed as issue?).

@aaron.ballman
@JonasToth
Thanks, i agree too! I've prepared new revision with additional option to preserve old inspection behaviour.

Provide additional option to preserve current inspection behaviour

Thanks, now all should be fine!

In D41535#981928, @ilya-biryukov wrote:

This looks good, but we should add a test.
Should've noticed that before, sorry for the slowing this down a bit more. After the test is there, I'm happy to commit this change for you.

Some more nits were fixed.

Fixed code review remarks.

Fixed comment

In D41947#980298, @ilya-biryukov wrote:

Looks good. Do you have commit access or do you need someone to land this patch for you?

Moved logic to ClangTidyMain

Implemented test-case to check that BaseFS is actually used in ClangTool

Friendly ping

Second part here (provided default virtual filesystem argument to ClangTool constructor): https://reviews.llvm.org/D41947

FYI, i've create revision to support ivfsoverlay option in Tooling: https://reviews.llvm.org/D41594

@ilya-biryukov
Yes, this is exactly what i need.
Unfortunately, -ivfsoverlay in the compile commands works for the compiler invocation, but it doesn't work for tooling.
E.g. this call:

clang-tidy -checks=* <file> -- -ivfsoverlay=<yaml_file>

has no effect.

Hello @thegameg !
Is is correct that printable UTF-8 characters are also escaped by these changes?
For example, Clang-Tidy output now uses double-quotes for UTF-8:

@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

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

Thanks for the response @klimek !
Sorry for possible confusions.

Yes. I'm still confused why in this case clang-tidy @file -- would be expected to expand the response file? Am I missing something?

I think that we discussed use-case when @response files can appear in the compiler options (right after -- symbol).

I don't understand this. Can you elaborate?

As IDE developers we can't control how users pass options to compiler.
For example, user can set compiler options directly in the CMake, like this:

If you want one unified format, the compilation database is it.

Is the clang-tidy ... -- <args> meant to be more or less a drop-in replacement for clang <args> (arguments-wise)?
If yes, this expansion of response files here is an another step in this direction.

Are there any concerns using the alternative?

I can't say that it's a big problems, but i think that:
CompilationDatabase.json is more CMake specific format.
It can be generated automatically by CMake, while other build systems may not do it.
So we need to generate it on the fly (by tool or by hand), which also can lead to hidden problems due to different formats, different escaping rules, etc.
I think it's always good to have one unified format.

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

Thanks @klimek for the response!
Let me give an example.
Suppose we want to check main.cpp by clang-tidy.
As i said before, i cannot use CompilationDatabase.json (due to some technical reasons),
but there is way to pass all options though command line and call external process of clang-tidy in this way:
clang-tidy -checks=* main.cpp -export-fixes=... -- -std=c++11 -Ipath/to/include -Ipath/to/include2 -DMACRO ....
All compiler related options appear in the command line after -- symbol.
But due to command line length restrictions (especially on Windows OS) it's better to use @ResponseFile mechanism.
To achieve it, i've created arguments.rsp file with this content:
-checks=* main.cpp -export-fixes=... -- -std=c++11 -Ipath/to/include -Ipath/to/include2 -DMACRO ....
and tried to call clang-tidy in this way:
clang-tidy @arguments.rsp
But all compiler options are ignored by this call!
If you check how CommonOptionsParser.cpp parses command line arguments, you will see, that first of all, it tries to load fixed compilation database (by finding -- symbol in the command line).
Only after that it calls cl::ParseCommandLineOptions which loads content of arguments.rsp file and parses all the rest arguments.
So, my question was why not to expand response files before loading fixed compilation db from command line?
Or i'm doing something wrong?

To discuss:
This patch is required for correct integration Clang-Tidy with CLion IDE.
By this moment, we unable to use CompilationDatabase.json from CLion side which is widely used in Clang-Tidy and in other common tools.
Anyway, there are possibility to pass compiler options directly through command line.
But we cannot pass compiler options directly through command-line, due to command-line-length restrictions.
So, we decided to pass compiler options through @ResponseFile mechanism.
But there are some problems with this approach.
Before this patch, /clang/lib/Tooling/CommonOptionsParser.cpp worked in this way:

1. Load compilation database from command-line
2. Expand response files
3. Parse all other command line arguments

I think it's strange behavior?
Why we try to load compilation database first?
Maybe it's better to expand response files and only after that try to load compilation database and parse another options?
It's hard to refactor cl::ParseCommandLineOptions and cl::ExpandResponseFiles functions, due to high number of usages, so i copied some block directly into /clang/lib/Tooling/CommonOptionsParser.cpp
I don't think that is a best solution, but i don't have another one.

• trailing period
• moved test-cases to separate file

This is second part of review:
https://reviews.llvm.org/D34404
which is contains changes related to clang-tools-extra repository.

I've splitted single revision into two different revisions:
https://reviews.llvm.org/D34404
https://reviews.llvm.org/D35349
Also, i fixed failing test-cases in clang-apply-replacements due to changes in output format (so they shouldn't be a problem anymore)

1. split revision into two
2. fix failing tests

• made code less verbose
• used assignment initialization form
• removed extra .c_str() call

Thanks, Alex for your suggestions.
I agree with your remarks, but i thought it's only personal style preferences.
I'll fix it soon.

• marked some arguments as const
• use {} instead of explicit variable declarations

• use EXPECT_* instead of ASSERT_* where it's possible
• Diagnostic constructor now takes const references for it's arguments
• removed extra namespace qualifiers

• moved test-case from separate file to existing one
• fixed No newline at end of file problem and put space inside comment

• fixed No newline at end of file problem
• provided test-case to check that diagnostics with no fixes will be applied correctly

correct revision with all changes

updated CMakeLists.txt