- User Since
- Jul 12 2018, 2:31 PM (36 w, 2 d)
Thu, Mar 21
I was not able to come up with a test that would detect this issue using either clang-import-test nor via any of the methods used in ASTImpoterTest.cpp. I created a regression test on the lldb side, which should pass once this is committed:
It looks like this commit introduced undefined behavior via a misaligned load see this log from lldb sanitized bot on green dragon
Mon, Mar 18
Fri, Mar 15
Wed, Mar 6
LGTM although there are some good comments by @aprantl
Tue, Mar 5
LGTM and I don't see any regressions.
LGTM outside of the question I had.
Mon, Mar 4
LGTM, thank you !
Fri, Mar 1
Thu, Feb 28
It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.
Addressed comments on formatting and missing changes to the Import_New version of the code.
Wed, Feb 27
Tue, Feb 26
LGTM I don't see any LLDB test regressions but I would prefer another reviewer as well.
Feb 15 2019
LGTM, I just ran check-lldb and I don't see any regressions.
Thanks for fixing this.
Feb 14 2019
Looks like a great fix!
Feb 12 2019
I ran check-lldb locally and it looks good.
Feb 7 2019
Feb 6 2019
This looks reasonable to me but I don't have strong opinions on refactoring gtest tests.
LGTM although I wish we had more lambda tests.
Jan 31 2019
Jan 30 2019
@martong have you had a chance to look at this some more? We ran into another problem that this fixes partially. Can I help in any way?
Jan 29 2019
Addressed comments, including
- Renamed test
- Reduced test size
- Stream-lined code in CreateTemplateParameterList(...)
Jan 28 2019
Addressing comments on commenting and naming.
Jan 25 2019
Nice improvement in readability!
I don't believe it is directly related to modules but that it just triggers the issue because it may be bringing in more. You can find a description here https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-g and @aprantl pointed me to this talk if you want to even more http://llvm.org/devmtg/2015-10/#talk19
Jan 24 2019
@martong Unfortunately this causes a regression on one of the lldb tests TestDataFormatterLibcxxVector.py e.g.
@martong so I just tested your lit-tests.patch and it looks good!
Changes based on comments
- Refactoring ImportFunctionDeclBody to return Error
- Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
- Removing merging of function body for later PR
Jan 23 2019
Addressing comments on naming in the unit test and refactoring of duplicated code.
@martong I don't follow the discussion on VisitParmVarDecl an what seems like an alternate fix. Can you elaborate?
Jan 18 2019
@martong the only issue is that I am seeing a regression on Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but if you have any insights that would be helpful.
Jan 16 2019
Thank you, this looks good but perhaps some refactoring would help improve the change.
Jan 15 2019
Sorry for the late review
Jan 11 2019
Can you add a test?
Thank you for adding the additional test.
Dec 12 2018
LGTM after comment are addressed.
Dec 11 2018
I find the static_cast<bool> to be a bit too clever, I don't think it helps readability.
Dec 10 2018
See changes here
The fix was not too bad, I landed the fix and the bots just turned green http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/14015/
Just a heads up this change broke the lldb std::function formatter see the logs here:
Dec 7 2018
Dec 6 2018
This PR broke the green dragon bots, see the following log file:
Nov 28 2018
As pointed out in this comment in another review
Apologies, I meant to make the comment in the child PR