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.

Addressed missing colon in FIXME comment.

I added the FIXME and reduced the amount of testing. Let me know if it looks alright.

Address comments: reduce testing & add FIXME.

When compiling this I get the following warning:

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1213:10: warning: enumeration value 'CXXAddrspaceCastExprClass' not handled in switch

LGTM

Please see https://reviews.llvm.org/D55136 for the patch addressing these issues.

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

In D52879#1311177, @riccibruno wrote:

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

In D52879#1309734, @riccibruno wrote:

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

LGTM, but please add a comment in the test file.

ping

Note: the get_kernel_* functions used here all return unsigned integers, hence the warning/present patch.

In D52875#1255146, @erichkeane wrote:

Can you write tests for this please? Particularly validate the results in a constexpr context.