LGTM, thanks for the update.

Some minor comments.

Some relatively minor comments, overall direction seems good to me.

looks appropriate to me.

I feel this would be a valuable addition, indeed. Only a minor question from me.

LGTM, thanks. All the comments seem to be addressed.

Nice refactoring & fix! LGTM. I suppose Stuart's comment about moving typeNameRef can be addressed when pushing.

Right, thanks for the explanations. They make sense.

Looks sensible to me.

LGTM

Looks good overall.

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?

LGTM

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.

lgtm, thanks

Looks good, only a few typos and minor comments.

When reading the documentation [1] 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.

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.

I've put up https://reviews.llvm.org/D90385 as a fix for this issue.

A follow up on https://reviews.llvm.org/D60193.

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.

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.

Addressed comments.

In D89372#2329939, @yaxunl wrote:

what if users rely on the predefined macros associated with the extension e.g. cl_khr_srgb_image_writes to enable/disable certain code?

What's the issue with these extensions not removed?

LGTM

Thanks for your clarifications and updates. Just one tiny question about a test file, but otherwise LGTM.

Alright, thanks for the explanation. LGTM then.

Shouldn't this page be referenced from clang/docs/index.rst?

In D67753#1681049, @thakis wrote:

That's mysterious indeed. Looks like the default warnings come from here: https://github.com/python/cpython/blob/88e6447451fb5525e83e802c66c3e51b4a45bf86/Python/initconfig.c#L2064 You don't happen to run python in dev mode, do you? In dev mode, the warnings default to "default"; else they apparently don't.

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. :/

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.

Let me know what you think of this patch. Thanks.

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?

Thanks for addressing my comments.

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.

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.

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.

In D64569#1592059, @rjmccall wrote:

Yes, that's the right fix, although you might also consider adding a getObjectType() to CXXMemberCallExpr.

• Add minimal regression test for PR42665
• Add CXXMemberCallExpr::getObjectType()

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

• Addressed comments

Beside my two comments, I think this looks good.

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).

Here are a few comments from me but keep in mind that English is not my primary language

I'll commit this along with D64083 in a few days in case John has any feedback.