User Details
- User Since
- Jul 10 2017, 11:37 AM (289 w, 6 d)
Thu, Jan 26
Wed, Jan 25
Thanks for the reviews!
Wed, Jan 11
Tue, Jan 10
Dec 16 2022
I came across a strange error when capturing arguments in a lambda inside another lambda. I filed an issue here - https://github.com/llvm/llvm-project/issues/59549
Nov 18 2022
Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?
Feb 7 2022
Feb 4 2022
Jan 13 2022
Jan 12 2022
Implemented review comments
- Remove unnecessary set method and set default value during construction instead
- Changed default address space for function pointer for avr target to 1.
Jan 11 2022
Add program address space to TargetInfo. Targets with non-default address space for functions should explicitly set value.
Jan 10 2022
Implement review comments - add a comment + remove unnecessary code
Implemented review comment to move logic into function (getTargetAddressSpace) as opposed to call site.
Jan 7 2022
Dec 2 2021
Thanks for the reviews @dylanmckay and @rjmccall ! I agree that moving the logic for functions pointers to getTargetAddressSpace makes sense. However, I'm not sure what the consequences are, since that increases the impact of this change quite a bit. I'm not sure if I will have the time to deal with any issues that arise before I go on vacation for Christmas. I'll take a quick look sometime this week, and hopefully its a simple enough change. If not, I can do it in a follow-up PR as suggested above, unless someone objects.
Nov 30 2021
Since this patch has been accepted by one code owner, and has been under review for over a month, I plan to submit it by the end of the week. If anyone has feedback/concerns, please comment on the review.
Nov 9 2021
This patch causes a regression.
Nov 4 2021
ping
Nov 2 2021
Nov 1 2021
Oct 29 2021
This has changed the behavior for -
Oct 21 2021
Oct 20 2021
ping
Oct 13 2021
Thanks for taking a look @yaxunl! I added a test.
Oct 11 2021
Mar 18 2021
Mar 8 2021
Implemented review comment to restrict __stdcall default calling convention (for WinMain, wWinMain, and DllMain) to 32 bit Windows.
Mar 4 2021
Feb 12 2021
LGTM Thanks!
Sep 17 2020
Sep 16 2020
Sep 15 2020
Feb 12 2020
Actually I just noticed it still says 'Needs review' above. I'm not sure what the protocol is. Can I push the patch?
Thanks! I'll push it shortly. Just running a final ninja check-all after updating workspace.
Feb 6 2020
Thanks for taking a look Richard. This patch adds the required value-dependency check and sets type-dependency accordingly.
Jan 22 2020
Jan 9 2020
Semantic analysis for value dependent conditions is now skipped as per Erich's comment. Patch adds an argument to CheckBooleanCondition to still do the required analysis for noexcept expressions.
Jan 8 2020
Thanks for taking a look Erich!
Jan 5 2020
Dec 5 2019
Dec 3 2019
Nov 19 2019
Nov 11 2019
The patch changes type dependency of field 'std::string s' and sets it based on field type (i.e. not type-dependent).
Nov 8 2019
Thank you for letting me know. The patch has been reverted.
@rnk @gribozavr2 clang-tidy test "bugprone-string-integer-assignment.cpp" caused a bot fail on windows. From the logs it looks like the 'CHECK-FIXES' I added in that test is failing because the fix isn't applied. I am not sure why. I don't have a windows build set up and I assume a new build is going to take some time. I am not sure what the protocol for this is. Should I go ahead and revert my patch while I investigate the issue on Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I look into the issue?
Thank you for taking a look Reid and Dmitri. There were no fails with check-all.
Nov 7 2019
Aug 14 2019
Thank you for letting me know. I did try reproducing the issue with check-all yesterday but was unable to. I do not think I build with 'clang-tools-extra' project enabled though. I'll take another look today.
Aug 13 2019
Aug 7 2019
Thanks for taking a look Reid! I rebased the patch and had a conflict with one of Richard's patches and so ran the lit tests again and noticed there are several new failures. I am taking a look at those now.
Jul 29 2019
ping
Jul 17 2019
I just realized I modified the summary when I uploaded a new patch, but did not add a comment about the changes I made. Based on feedback from the bug report (https://bugs.llvm.org/show_bug.cgi?id=40982), I changed my approach for this crash. This patch changes the type dependency of the member which causes the crash.
Jun 10 2019
Apr 24 2019
I ran the test you provided and it does throw errors without instantiation
Apr 23 2019
I apologize for the confusion. I was working on an incorrect branch earlier, and so abandoned that patch and uploaded a new revision.
Nov 6 2018
Nov 2 2018
Thanks!
Nov 1 2018
Upstreaming it first might be better, especially since the change seems to be trivial. Is this line addition the only change proposed for the benchmark library?
Yes this line is the only change.
Do you want me to submit the patch to the benchmark library? It seems that it would help to speedup the process.
That would be helpful. Thank you!
@kbobyrev I apologize if I was unclear in the comments. I was asking if the changes proposed in the comments are alright with you since they would involve modifying benchmark/CMakelists.txt (instead of llvm/CMakeLists.txt as discussed in mailing list). As Zachary mentioned in comments, _HAS_EXCEPTIONS should be set to 0 only when exceptions are disabled. Since exception handling for benchmarks is handled in benchmark/CMakeLists.txt, I think it makes most sense to add the definition there. I have now uploaded the proposed change for review.
Oct 29 2018
Oct 25 2018
I think it makes sense to include this case just to make the flag more complete. Also, we don't really match GCC behavior for this flag. GCC emits all static constants by default (when O0). When optimized, this flag has no effect on GCC. The negation is used to not emit the constants.
Oct 9 2018
Yes. I understand. At the moment, exception handling flags are generated based on BENCHMARK_ENABLE_EXCEPTIONS in utils/benchmark/CMakeLists.txt . However, _HAS_EXCEPTIONS is not defined based on this (code below). The warnings are a result of this mismatch.
Oct 8 2018
Thanks for the information Zachary.
I do not think defining _HAS_EXCEPTIONS=0 affects C++ exceptions in the application. I thought it only affected the STL. I will verify this and update you.
Sep 4 2018
Alright. Thanks for the review. I will abandon this patch
Aug 31 2018
@lebedev.ri is there a specific reason -Wtautological-unsigned-zero-compare was removed? All the issues I am aware of talks about comparisons with min/max.