- User Since
- Oct 1 2018, 7:44 AM (132 w, 11 h)
Mar 2 2021
LGTM, thanks for the update.
Feb 24 2021
Some minor comments.
Some relatively minor comments, overall direction seems good to me.
Feb 23 2021
looks appropriate to me.
Feb 16 2021
I feel this would be a valuable addition, indeed. Only a minor question from me.
Feb 15 2021
Feb 11 2021
Feb 9 2021
LGTM, thanks. All the comments seem to be addressed.
Feb 8 2021
Nice refactoring & fix! LGTM. I suppose Stuart's comment about moving typeNameRef can be addressed when pushing.
Jan 27 2021
Right, thanks for the explanations. They make sense.
Looks sensible to me.
Jan 8 2021
Jan 7 2021
Looks good overall.
Jan 6 2021
Thanks for the update.
LGTM, except for a minor typo. Otherwise my comments have been addressed. Thanks.
LGTM, just one question: I see in the other review you updated clang/test/SemaOpenCL/extension-version.cl. Do you need to do the same here?
Dec 31 2020
Thanks for the update. Looks good overall. A minor question about backticks. I'm no native English speaker, so I would recommend a second review from someone else too.
Dec 30 2020
Looks good, only a few typos and minor comments.
Nov 26 2020
When reading the documentation  for -cl-ext (which I've never used so far), I've noticed nothing is said about non-standard configurations (such as disabling cl_khr_depth_images with CL2.0). Quickly testing this shows that options can be specified to produce non-standard behaviour, as shown by https://godbolt.org/z/1Yz1Md.
Nov 13 2020
LGMT as it reduces the divergence in compilation flows. I let @Anastasia, or someone else more familiar with the codebase, give the final approval though.
Nov 9 2020
Oct 30 2020
Oct 29 2020
I've put up https://reviews.llvm.org/D90385 as a fix for this issue.
A follow up on https://reviews.llvm.org/D60193.
Oct 22 2020
Oct 20 2020
I don't want to stop the wider discussion, that being said I think I've addressed the comment regarding the content of this PR. Let me know if the latest version is fine or needs further addressing. Thanks.
Oct 16 2020
Keep cl_khr_byte_addressable_store and cles_khr_int64 out of this PR. Add more details on the extensions being removed in the commit message.
Oct 15 2020
Oct 14 2020
May 11 2020
May 7 2020
Thanks for your clarifications and updates. Just one tiny question about a test file, but otherwise LGTM.
Oct 17 2019
Alright, thanks for the explanation. LGTM then.
Shouldn't this page be referenced from clang/docs/index.rst?
Oct 16 2019
Sep 25 2019
Sep 24 2019
I don't have PYTHONWARNINGS set in my env. Maybe ninja invoke the script using python -Wignore write_cmake_config.py or something? It's quite worrisome that Python runtime can exhibit different behaviours without apparent reasons. :/
/usr/bin/env python --version Python 3.7.0
I'm afraid I can't give you the exact log we have, because we have a special integration of LLVM downstream and don't directly rely on the usual cmake/gn/... workflow. I'm sorry about that.
You need Python 3.7.4 or later. The warnings are emitted only starting from this minor release.
Sep 19 2019
Let me know what you think of this patch. Thanks.
Sep 3 2019
Aug 21 2019
I think this looks good. Maybe the tests should be extended to test auto as function return type, and if there's some special handling around decltype(auto), then it should be tested too, but I'm not sure it's actually needed here. What do you think?
Aug 19 2019
Thanks for addressing my comments.
Aug 15 2019
The overall structure seems alright. I'll let people more familiar with the code-base judge whether other important features should be added there as well.
Jul 30 2019
LGTM, thanks for the update.
In vector_literals_nested.cl, we have tests for (global) constants. Do you think it would be worth testing those as well in C++ mode? Maybe the two files (vector_literals_nested.cl and vector_literals_valid.cl) should also be merged as most of their content seems duplicated.
Jul 22 2019
FYI In order to fix buildbot test failures, I've pushed https://reviews.llvm.org/rG1b2da771f561affe36eb5eb0c7a3d2862c5a5c1c. I'll keep an eye on buildbots for additional fallout.
Jul 19 2019
- Add minimal regression test for PR42665
- Add CXXMemberCallExpr::getObjectType()
Jul 18 2019
Add patch for Bug PR42665.
While investigating PR42665, I've noticed that getImplicitObjectArgument doesn't always return a pointer type. For example, when dealing with shared_ptr. In libc++'s test std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp, dumping CE in EmitVirtualDestructorCall shows that the callee is .~Bar (mind the .):
CXXMemberCallExpr 0x29d10a0 'void' `-MemberExpr 0x29d1070 '<bound member function type>' .~Bar 0x23f5250 `-CXXMemberCallExpr 0x29d1040 'struct Bar':'struct Bar' lvalue `-MemberExpr 0x29d1010 '<bound member function type>' .second 0x2883328 `-MemberExpr 0x29d0f80 '__compressed_pair<class std::__1::allocator<struct Bar>, struct Bar>':'class std::__1::__compressed_pair<class std::__1::allocator<struct Bar>, struct Bar>' lvalue ->__data_ 0x2896960 `-CXXThisExpr 0x29d0f70 'class std::__1::__shared_ptr_emplace<struct Bar, class std::__1::allocator<struct Bar> > *' implicit this
- Minor refactoring of getThisObjectType
- Removed unnecessary assertion
Jul 17 2019
- Addressed comments
Beside my two comments, I think this looks good.
Jul 16 2019
This should address the minor refactoring requesting in D64083.
Mind the fact that I've rebased the changes onto a more recent master version. If you look at the diff of v1 against v2 you might see some unrelated changes.
- Refactored common bits from CheckConstructorDeclarator and CheckDestructorDeclarator.
- Added as many "ThisTy" parameter I could.
- Addressed issue with identifier format in tests (%N to %var.ascast).
Jul 12 2019
Jul 11 2019
Jul 9 2019
Here are a few comments from me but keep in mind that English is not my primary language