- User Since
- Mar 23 2016, 8:38 AM (73 w, 4 d)
Thu, Aug 17
Wed, Aug 16
That's a really nice approach to this problem, good job Yuka! See my inline comments for some minor remarks.
Tue, Aug 15
Very well done, I really like this patch! I added a few remarks mostly about the comments that need some small adjusting.
Thu, Aug 10
LGTM, more publicity means that more shells will make use of it.
Thu, Aug 3
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.
Tue, Aug 1
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=.
Mon, Jul 31
- Remove some unneeded includes
Sun, Jul 30
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?
- Updated according to Artem's comments.
Tue, Jul 25
If you can't reproduce, you should try running a debug build through valgrind. It points out this issue:
Sat, Jul 22
Thanks for the quick fix, LGTM!
Jul 18 2017
ping. Any objections to adding this GCC alias?
Jul 15 2017
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).
Jul 14 2017
Jul 9 2017
@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).
Jul 2 2017
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 :)
Jul 1 2017
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?
Jun 30 2017
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.
Jun 29 2017
Two thought from me on this:
Jun 25 2017
Works as intended, good job!
Jun 24 2017
Jun 22 2017
Jun 21 2017
@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.
Jun 20 2017
Jun 19 2017
Jun 18 2017
LGTM, thanks for the patch!
Jun 16 2017
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!
Jun 15 2017
- 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.
Jun 14 2017
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...)
Jun 13 2017
Jun 12 2017
Jun 8 2017
- Just unconditionally calling setHidden now.
May 22 2017
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.
May 19 2017
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 :)
May 17 2017
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 :).
May 9 2017
Work in progress patch. We probably also need to update a few other places where we call this.
Apr 25 2017
- Now using unique_ptr instead of raw pointers.
Apr 23 2017
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.
Apr 21 2017
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.
Apr 19 2017
Apr 17 2017
Woops, my bad, seems like I tested just before you fixed it.
Apr 16 2017
@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@/../.
Apr 11 2017
- Readded the missing tests to get the final diff.
Apr 10 2017
- Fixed compilation error in the cleaned up version (removed two lines too much...)
Apr 6 2017
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.