LG from my side.

lg

lg

LG, as discussed in person, it's probably a good idea to try to get rid of the non-file-creating version, if possible, or at least fix the comments on the functions to make this behavior clear.

LG

In D35955#835994, @euhlmann wrote:

In D35955#835439, @klimek wrote:

I think if we need this info, we can just make it count down to -1 again (or, but that's isomorphic, let it run from 0 and make sure we never index into the data structures at 0 :)

Should I do one of these things? Let me know how you'd like me to implement this.

In D35955#834914, @djasper wrote:

Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1?

Also missing tests :)

High-level question: Why can't we use llvm::ThreadPool?

lg

lg

In D36397#833890, @ilya-biryukov wrote:

In D36397#833883, @klimek wrote:

Tests?

TSAN does not catch this (as it's a logical error) and it requires a rather bizarre timing of file reparses to trigger.
I couldn't come up with an example that would reproduce this.

Tests?

Sorry for missing this - can you add a test?

Thanks! LG after comment change.

Just as a general note: adding cfe-commits after the fact is usually not a good idea, as we lose the history of the review in the email list (which is the source of truth).

Why? Also, missing tests.

+Ben who I believe had written a similar function once.

ptal

Address review comment.

Nice catch! lg

Apart from nits, looks OK to me - Eric, are all your concerns addressed?

Adding Ben as reviewer.

Update to handle hasDeclaration for type alias template decls.

lg

Could we perhaps only suppress the warning when we shadow a variable within the same declcontext? Generally, with macros, shadowing local variables is more surprising.

Your patch description sounds like you're only switching the warning off when the scope ends in a macro, but the patch looks different. For example:

#define M int x

In D34440#812938, @vladimir.plyashkun wrote:

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.

lg

In D33644#812693, @yvvan wrote:

So do we wait until the '=' case is more clear? This change should not break anything if it's fixed in either direction (if '=' will be provided always or never)

Submitted as r308290.

In D34440#809581, @vladimir.plyashkun wrote:

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.

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

That worked?

In D34440#808156, @vladimir.plyashkun wrote:

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.

lg

lg

+Richard, as apparently we get the source ranges for default values of built-in types without the preceding "=", but for user-defined types including the preceding "=" - is that intentional?

Yep, I want Richard's approval for the name. I think he already expressed a pro-pulling-out stance.

LG

Specifically, ping Richard for new top-level lib in clang.

lg

In D35066#802081, @mteodor wrote:

In D35066#802073, @klimek wrote:

are you making changes to the generated file, and want to not format those changes?

Yes. Sometimes we change IDs on all code including resource.h, but for this specific file
we want to keep the formatting set by Visual Studio resource editor.

In D35066#802066, @mteodor wrote:

In D35066#801955, @klimek wrote:

Looking at the code, probably the dirty check should be done sooner than later:

if (!options.FormatOnSave || !Vsix.IsDocumentDirty(document))
    return;

Where are you seeing this? In ClangFormatPackge.cs it looks like we're returning if the document's not dirty?

This was just a proposed change for ClangFormatPackge.cs.
I think it makes sense to move this check above the file extensions check because the later is useless if the document is not dirty.

Of course, I just assume that IsDocumentDirty() call is cheaper than FileHasExtension() + FileIsIgnored() calls.

Let me know if you would like to upload a new diff than contains this small optimisation too.

In D35066#801888, @mteodor wrote:

In D35066#801878, @mteodor wrote:

No, this was not changed and works just fine. Format on save triggers only if there are changes.

Looking at the code, probably the dirty check should be done sooner than later:

if (!options.FormatOnSave || !Vsix.IsDocumentDirty(document))
    return;

Assuming you're not changing / saving a generated file, I'm wondering whether the real bug is that format-on-save triggers if nothing in the file changed?

In D34512#800626, @whisperity wrote:

In D34512#800502, @xazax.hun wrote:

In D34512#800499, @klimek wrote:

In D34512#800490, @xazax.hun wrote:

It looks like Richard approved libTooling as a dependency for clang on the mailing list (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html).
If it is ok to have this code in libTooling (for now), I think we could start/continue the review of this patch.

I read that somewhat differently? It seems like Richard basically proposes adding a new library for things that control how we run clang in a multi-TU scenario. I'd call it libIndex, but that already exists :)

No, but since Richard said he did not see any justifications to include this in libTooling, I had the impression this is not his final word, and in case you see it justified, this remains the suggested direction.
But in this case I will rewrite this patch to create a new library. Are you still interested in reviewing it? Do you have any other name in mind? What about libCrossTU?

In D34512#800625, @xazax.hun wrote:

In D34512#800618, @klimek wrote:

+Richard as top-level code owner for new libs.

For bikeshedding the name: I'd have liked libIndex, but that's already taken. CrossTU doesn't seem too bad to me, too, though.

Some brainstorming for the bikeshedding: libProjet, libProjectIndex, libImport
The reason I am not sure about the Index in the name because this code does not contain (yet) an interface to create/handle indexes.
The import might be a good once, since this library is basically wrapping the ASTImporter to import code from external sources, but it might be misleading, since ASTImporter is in the AST module.

Cross-TU is now used (in the lifeline of these features introduced by the team) in multiple contexts: CTU analysis, and now CTU lookup.
Maybe the name of this lib should reflect on the fact that the lib is used to support cross-TU definition lookup.

Hmm... libCrossTULocator?

+Richard as top-level code owner for new libs.

lg

In D34512#800490, @xazax.hun wrote:

It looks like Richard approved libTooling as a dependency for clang on the mailing list (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html).
If it is ok to have this code in libTooling (for now), I think we could start/continue the review of this patch.

LG

In D34197#799062, @mehdi_amini wrote:

In D34197#798738, @klimek wrote:

In D34197#795442, @mehdi_amini wrote:

Thanks! LGTM.

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

As a data point - I just tried to build the docker image and without Ilya's help I wouldn't have known to look at the docs (usually docker build envs for projects just work out of the box to give me a full image; I don't know whether you have a different experience). We'll improve comments on the script itself, but as a user I'd really like to have something that I can run that will Just Work :)

I don't disagree that having something that "just works" would be nice. However you have to take into account that LLVM is a very complex project and there is not a "single image" that can fit all.

If we start with an "easy to build" full image, it should be the same as the official packages: http://releases.llvm.org ; this is *not* what was proposed here.

In D34197#795442, @mehdi_amini wrote:

Thanks! LGTM.

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

lg

LG, thx for bearing with me, this looks great.
(and sorry if I didn't send this earlier, just noticed I forgot to hit submit :( )

In D34696#795020, @arphaman wrote:

In D34696#793613, @klimek wrote:

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

Would it be better if I make a patch for the clang-refactor tool first and then update this patch to support some of clang-rename's functionality in clang-refactor and move the tests over? Or should I just move all of clang-rename and its tests to clang's repository first and then focus on clang-refactor?

lg

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

In D33644#793601, @yvvan wrote:

In D33644#793594, @klimek wrote:

In D33644#793577, @yvvan wrote:

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Taking your example "const Bar& bar = =" when Bar is not defined:
What happens now? Will it be "const Bar& bar ="? I argue that is not better than "const Bar& bar = =".
Btw, can you add tests? I think that will make it easier to see what's actually happening.

It will be just "const Bar& bar" in that case with this patch applied. So it is better than both "const Bar& bar =" and "const Bar& bar = =" :)
About tests - I agree that it's nice to have them. Can I find something similar in the current test set (as an example) and where is it better to put my tests?

In D33644#793577, @yvvan wrote:

In D33644#793573, @klimek wrote:

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

In D33644#783903, @yvvan wrote:

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

lg

Richard (added as reviewer) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good.

Adding folks interested in the indexing discussion.

In D34304#788080, @saugustine wrote:

In D34304#787699, @klimek wrote:

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

This gets back to why the original patch plumbed the boolean all the way down to newInvocation.

newInvocation is the function that actually discards the dependency file options--today unconditionally.

General direction looks good.

LG. When you submit, please note the other fixes in the commit message (or make it 2 commits)