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

See my inline comments about technical changes, but otherwise this looks ready to land. Please update and I'll give green light ASAP. Thanks!

• made saveHash static.

Sorry for the delay, we got stuck because hard coding a certain file pattern into the clang source code doesn't seem to be a optimal solution (e.g. every user that has a different set of generated files needs to patch clang to hide his specific reports). I would prefer if we could get this into a variable that the user can change dynamically.

Everything beside this last test case seems to be handled. And from what I remember there was a longer discussion how to properly handle this case and so this review got stuck.

LGTM, good job! (Sorry for the delay, I think I got interrupted here by the GSoC start...)

• Just unconditionally calling setHidden now.

Good progress and good catch with also checking lld :)

• Now unhiding/unowning the created functions like Richard suggested.
• Extended the test case to stress test the lookup with the new visibility settings.

I'm not 100% sure if that's the right fix, but I didn't see a obvious way to declare the new/delete outside the current submodule. Maybe Richard knows a better way :)

Because the small follow up patch is relevant for the review of this one. If people disagree with the plan there to extend Options.td or OptTable for this feature (and due to the limitations of Options.td in regards to dynamic arguments, this is already controversial), then we have to completely revisit this code which assumes we can do everything in OptTable. I fully agree with incremental development in tree, but for those first two patches I don't see "consensus on the end goal to define the first increment" yet (to quote the LLVM dev policy on this). Sorry, I should have mentioned this fact in the previous mail, but typing on the smartphone is cumbersome.

@ruiu We wanted to push this patch with the follow-up one about the argument completion, so no need to do this now :).

Work in progress patch. We probably also need to update a few other places where we call this.

• Now using unique_ptr instead of raw pointers.

This also disables warnings for -MD and -MMD which shouldn't happen as we're actually compiling code here and are probably compiling a project. E.g. for people that use this mode to create dependency files while initially building (to figure out when to rebuild each object), this will effectively force -w to the whole build process which is not good.

I think most of the changes are just necessary because we replaced LLVM_MAIN_SRC_DIR with CMAKE_SOURCE_DIR (all the added ../ and removed docs). On the other hand this is now in sync with the way the the clang doxygen.cfg is structured and it fixes the include paths, so I think this looks reasonable.

Woops, my bad, seems like I tested just before you fixed it.

@yamaguchi Did you test the latest revision of this patch and get the desired output paths? I just tested it and it seems on my system doxygen can't handle the @abs_srcdir@/../.

• Readded the missing tests to get the final diff.

• Fixed compilation error in the cleaned up version (removed two lines too much...)

I think for range loops work differently:

• Renamed variable from 'S' to 'Child' to make the buildbots happy (and because it's more expressive)
• Fixed the name of the unit test.

Change it to a namespace when merging if you prefer that :). Thanks for the review!

Please fix these things:

• Remove the mysterious unicode-character at the start of CloneDetection.cpp
• Fixed formatting of the comment in CloneDetectionTest.cpp
• Fixed comment in StmtSequence::contains that was still talking about checking for translation units even though we compared declarations.

• auto to const auto to be more consistent with LLVM style.

• Remove the assert(ChildHash) which is obviously wrong code that just rarely fails.

Well this patch won't change a lot with the false-positives or performance (it's more refactoring) :)

Hey Leslie,

Thanks for the review Artem!

The constraints should be able to do the same filtering with the same performance in a cleaner way. So once D23418 is merged, we can put this code into a constraint and just prepend it to the list of constraints we use in the checker.

• Removed the work-in-progress changes to: loadDeclUpdateRecords
• Added FIXME that this function should be patched too.

• Prefixed PR31863.cpp with something meaningful.
• Added comment to delaly-emit-check.cpp to explain what we test there.

• Renamed InterestingDecls to PotentiallyInterestingDecls to better reflect its purpose.
• Ran patch through clang-format (now even with the correct code-style) and did the other requested style fixes.
• Added test from PR32186.

I just tested this on Mageia and this patch is superseded by now (it was originally written in 2013). The latest clang release is already able to correctly handle Mageia.

• Removed all the deprecated \briefs

I feel like we have a really similar use case in the Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried substituting the call to the statement hashing with a call to the CloneDetection API and it seems that most tests pass and the remaining fails are just minor fixable differences (like ::foo() and foo() having different hash codes).

Looks good to me. I'll add Richard because he also merged/approved the original post-order patch.

• I ran all real-world tests (sqlite, etc.) before rebasing to trunk. I'm not 100% confident that I correctly merged everything, so I'll rerun them just in case. The normal clang test-suite passes, so it looks good.

• Rebased patch to the current trunk state.
• Replaced runtime polymorphism with templates.
• Constraint interface now only has one method signature.

• Replaced hash_stream with MD5 in the sub-sequence code.

• Fixed typo.

• Rebased patch.
• Renamed passes to constraints.

Thanks for the tips and the review!

• Moved from hash_stream to LLVM's MD5 implementation.

• Added false-positives note for empty macros to the test suite.

• Added more documentation to the CloneSignature::Complexity field.
• Macros now have a complexity value of 1 + sum(ChildComplexityValues).
• Tests should be less cryptic now.

The immediate motivation is that in D22515 we need to generate a hash code for data that isn't in a container but implicitly stored in the properties of some AST nodes. This hash code needs to be generated for every single node in the AST so we try to avoid calling any heavy code during that. But to generate a hash code with the current API we first need to forward our data stream that we get from the visitor to a container like FoldingSetNodeID (which could even allocate memory) and then forward that container to the hashing function which handles it again like a stream of data.

This intention of this patch wasn't to extend the proposed API in any way; the only reason why the hash_stream class landed here is because LLVM's CityHash implementation is in this file's ::detail:: namespace and implementing hash_stream somewhere else would mean depending on the implementation details in different source files. Sorry about the confusion.

• Made warning messages more 'clangish' as pointed out by Vassil (Thanks!)
• Improved the class/variable names as pointed out by Artem (Thanks a lot!)
• CloneChecker's functions are now out-of-line.

• As the hash_stream patch will take a while to land upstream, we will change this to use FoldingSetNodeID and change it to hash_stream once that landed.

• Renamed FoldingSetWrapper to FoldingSetNodeIDWrapper
• Added missing // end anonymous namespace