• Rebased with the correct diff...

• Rebased diff before merging.

• Moved code from FrontendAction.cpp into createFileManager()

• Added test case.

@johannes The blocking reviewer is because it touches clone detection code :) Fine with me, I have some comments on things but nothing that affects this review. LGTM!

• Rebased and updated patch before merging.

• Rebased and fixed merge conflicts before merging.

@arphaman I suggested tablegen in D36664 because I remembered we had some CMake sanity check about not having an *.inc in our include dir: https://github.com/llvm-mirror/clang/blob/master/CMakeLists.txt#L266 Not sure if it actually fires for our case, but it looks like it does.

LGTM!

Sorry for the delay, was on vacation.

Found one more minor comment typo. And could you do your changes to OptParserEmitter.cpp all in this patch? Because Rui/Me pointed out those things on this review, so this patch should also fix them IMHO :)

That's a really nice approach to this problem, good job Yuka! See my inline comments for some minor remarks.

Very well done, I really like this patch! I added a few remarks mostly about the comments that need some small adjusting.

LGTM, more publicity means that more shells will make use of it.

I see, thanks for the information! If we don't need to support this in the shell-completion and we shouldn't report invalid arguments, then it seems this review is stuck here. I'll abandon as I don't see any other use case for this code.

E.g. If you add a flag that lands in between the results it breaks. Add -Wmajor-new-feature and the test goes red. Same with the flag results or changing the help test for -std=.

In D36067#825867, @xazax.hun wrote:

I like the directions of this patch.
In general, I am in favor of explicitly registering the options from user defined checkers.
But changing a config option will now break the command line compatibility, so I wonder how do we want to handle this:

• Have a list of no-op configs that we accept but warn that it has been replaced/removed?
• Just do not care and break compatibility?
• Something else?

• Remove some unneeded includes

I'm open to suggestions how we should handle user-provided checkers that maybe come with their own configs. Should we add that if we have a custom checker in the registry, that we disable this check to keep backwards compability? Or do we demand users to register their own configs in the registry?

(Fixed diff)

• Updated according to Artem's comments.

If you can't reproduce, you should try running a debug build through valgrind. It points out this issue:

LGTM.

Thanks for the quick fix, LGTM!

ping. Any objections to adding this GCC alias?

I have a last request in my inline comment regarding the documentation, otherwise this is good to go. Nice work!

@yamaguchi yes, the reason why we have to treat the -W... flags specially should be documented (as they're not in the OptTable as you said). E.g. something like // We have to query the -W flags manually as they're not in the OptTable. and then maybe a TODO: Find a good way to add them to OptTable instead and them remove this code..

It seems the code doesn't compile in clang because on your platform std::vector is implicitly included by some header, on clang+Arch however it isn`t (see the error at the end of https://teemperor.de/ccir/D35447).

@johannes D34880 has landed, so feel free to propose patches to the StmtDataCollector API that would help you (e.g. to support identifiers). You can see examples how to use it in the CloneDetection.cpp (once for storing data in a FoldingetSetNodeID and once for directly hashing the data with MD5).

Yeah, we should move it, but it should land somewhere in a header in /AST/ so that for example the StmtProfiler could make use of this. I'm open for suggestions here :)

For the record, one can get a (maybe incomplete) list of options that are without completion via: curl "https://teemperor.de/pub/clang_flags.txt" | grep -v HelpText | grep -v Group.
The only options I can see that I would miss if they would disappear are the ones who are clearly meant to be public but just lack the HelpText (like our beloved sysroot, which is a public GCC flag). So I think this patch can go forward and someone will have to add a HelpText for those flags in the future.

LGTM, everything still works in the latest bash on Linux.

I'm not sure what you refer to by I just saw we actually have the completion code in the Driver. We are already auto completing both cc1 options and driver options, right?

LGTM.

Yes, it does indeed skip identifiers and literals for this reason :). It was planned to make this template more configurable for use cases like yours, so I'm totally fine with adding configuration parameters. I just opened D34880 where I make this template public as a first step.

I didn't have time to have a close look at this patch, but it seems you're interested in the specific TU-independent data of a Stmt to compare them. So if you are interested in such data and don't want to write your own function to collect it for each Stmt subclass, there is the StmtDataCollector in the CloneDetection.cpp here and a example how to use it is here.

Two thought from me on this:

Works as intended, good job!

@ahatanak I think we can leave the more expressive clang name for this warning and just add the bit cryptic GCC name for compability. But I don't have a strong opinion on this.

LGTM, thanks!

LGTM.

LGTM, thanks for the patch!

Please check the last inline comment and then feel free to commit it with the suggested fix. And I wanted to wait for review on the other performance patches, so you can push this now.