- User Since
- May 27 2014, 6:39 AM (226 w, 1 d)
Fri, Sep 21
It seems like there are too many asserts and they are too specific, they seem to be aimed at specific potential bugs. What about asserts that make sure we maintain some invariants? For example, check DiagStr < DiagEnd once in a loop instead of every place we increment DiagStr. Do you think it should catch the same problems but maybe a little bit later?
Thu, Sep 20
Thanks for review.
Wed, Sep 19
The repro is
Reverted in r342599.
Seems like revert didn't go through. I suspect it is causing build failure in http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/49593/consoleText
I have published D52278 for disabling the test on Darwin. So far my investigation shows that __xray_log_select_mode("xray-fdr") returns 2 (aka XRAY_MODE_NOT_FOUND) and that's why we are calling std::abort.
Tue, Sep 18
Thanks for the fix.
Confirm that reverting the change locally fixes the tests. If nobody beats me to it, I plan to revert the change in 30-60 minutes. @srhines, if you want to fix it in another way and need more time, please let me know.
Seems like this change causes 2 test failures:
Mon, Sep 17
I've reduced the input causing error to
Who is responsible for updating the clients? Currently in the test suite MicroBenchmarks/XRay/FDRMode/fdrmode-bench.cc is failing to compile because it is using __xray::FDRLoggingOptions.
@dexonsmith, does my change address your concerns?
Looks like this changed causes errors in backend in stage 2 on Apple platforms
Fri, Sep 14
- Improve tests, fix -MMD, address comments.
Taking over the change, I'll address the reviewers' comments.
Thu, Sep 13
Mon, Sep 10
Agree that fatal/non-fatal error is too coarse and tooling/IDEs need more details and more control to provide better experience. But I don't think we are in a state to claim that all errors are recoverable (in theory and in current implementation). Instead of continuing on all errors, I prefer to select errors that are important for tooling and improve those first.
Rebased on top of trunk and checked that this is still working. Please take a look.
Draft is abandoned in favor of D50539.
Regarding the asserts to catch potential problems, seems most of them are for buffer overflows. Aren't sanitizers catching those cases, specifically Address Sanitizer? I haven't checked, just seems it would be good to check buffer overflow automatically instead of using explicit asserts.
Aug 20 2018
Thanks, Dmitry, for your explanation. It does help to understand your use case better.
Aug 17 2018
Thanks for review, Chris.
Aug 16 2018
Have you checked that produced AST is of sufficient quality to be used? Because, for example, for invalid code error recovery tries to keep going but the end result isn't always good.
Aug 14 2018
I don't have any other comments. Looks good to me.
Aug 13 2018
What about defining a feature for unsupported configurations? I've tried
Aug 10 2018
Thanks for the review.
Aug 9 2018
- No support for 'fallthrough' in crash reproducer.
- Current way of working with modules in VFS "root" is clunky and error-prone.
Aug 7 2018
Thanks for the review, Bruno.
Aug 3 2018
Eric, will you have time to commit this patch? If you don't have time and don't have objections, I plan to land this change on your behalf.
Aug 2 2018
Thanks for the review!
Aug 1 2018
- Address review comment: move getMainExecutable to indextest_core_main.
Jul 31 2018
Jul 27 2018
Looks good to me. Any further improvements are up to you.
Jul 25 2018
Thanks everyone for reviewing the change.
- Make diagnostics more general, use unit tests.
Jul 24 2018
Thanks for the pointer. Probably I'll move the added test there as it doesn't need full file system interaction and doesn't need to execute entire clang_cc1.
Jul 23 2018
- Tweak the comment according to review comments.
Jul 18 2018
Need to double check what tests we have when using relative path names at the root level. I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.
Jul 17 2018
Jul 16 2018
My review is incomplete, especially I cannot say with confidence if the proposed change is entirely free from unintended consequences that might break code not covered by the test suite. So other reviewers are welcome to chime in.
Overall looks good to me. Maybe add a test when a protocol is declared for an interface, not for a category. Something like
Jul 10 2018
- In C++03 call allocator's destroy when available.
- Rename _Args as it's not a variadic template pack.