I don't like the name getDependencies, because the function is not getting a list of dependencies, it's getting flags that indicate whether certain properties of the construct are dependent. Maybe getDependence or getDependenceFlags would be a better name? Likewise, instead of addDependencies, perhaps addDependence?

This seems reasonable to me.

In D73028#1839494, @steveire wrote:

In D73028#1839383, @rsmith wrote:

The follow-up is here: https://reviews.llvm.org/D73029 .

I have the need to change the ascending traversal implemented in ASTContext.cpp with a map (populated while descending) to make it skip nodes too during parent traversal.

I didn't want to have descending traversal in Expr.cpp and ascending traversal in ASTContext.cpp as they would be likely to go out of sync, so moving this implementation here allows extending the purpose in the follow-up.

We're working on moving the parent map out of ASTContext and into something specific to tooling; please don't add more dependencies onto it in the AST library.

An addition to the API will be concerned with ascending through the AST in different traversal modes.

This is not an appropriate fix; the code is locally correct. The right way to handle this is exactly what Clang currently does: if the expression is not type-dependent, then contextually convert to bool, and if it's not value-dependent, then make sure it's a constant expression.

In D67678#1836668, @dexonsmith wrote:

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

We can't comment on future releases.

That said, if this is urgent, let @arphaman know and we can try to qualify internally sometime in the next few months and we can report back what issues we find.

In D67678#1834957, @rsmith wrote:

I'll split out the new flag at least and re-land for Clang 10, while we figure out how to set the default.

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

In D72897#1832234, @vvereschaka wrote:

Hi Richard,

http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been broken by this patch during few days (failed "Clang::virtual-compare.cpp" test).
Sorry, but I'm going to revert these changes.

Mechanically, this looks fine.

I've left some comments suggesting how to rebase this on rGa42fd84cff265b7e9faa3fe42885ee171393e4db; otherwise, some minor changes then this looks good to me. Thanks!

• Explicitly put implicit virtual functions in the right order, rather

In D72897#1825791, @rjmccall wrote:

I know that Sema used to have a lot of issues with implicitly declaring special members, but I also know that that was bound up with how we tracked special state for the class, and that code has changed a lot. If you feel that that's an unlikely problem now, and you're comfortable with potentially having to generate implicit declarations of special members in different orders to satisfy different ABIs, I can accept this approach.

In D63744#1639938, @rsmith wrote:

Ping. Is there any interest in pursuing this, or is the preprocessor complexity too great?

I think this might work out more cleanly if we represented a constrained auto type as a ConstrainedType node wrapping an AutoType node rather than putting both things into the same object. (This will become more pressing if/when C++ starts allowing, for example, constrained placeholders for deduced class template specializations, or constrained typename types, which are likely future directions.) But let's go with this approach for now.

In D37933#872815, @davide wrote:

So, from what I can see no transformation made for free() is valid in case of operator delete.
What do you think instead of introducing a predicate isCallToOperatorDelete() and bail out at the beginning of visitFree() ?

Ping, I don't think I have any actionable feedback here and I'd like to get this landed before Clang 10 branches.

Rebase and slightly expand release notes.

Some cleanups (mainly parameters that are no longer necessary), then this is good to go. Thanks!

(Partial comments; I'll try to complete the review today or tomorrow.)

Thanks, I'm happy to iterate on this further after you commit (though feel free to ask for another round of review if you prefer). The changes to recovery from missing parens in a trailing requires clause can be done in a separate change after this one if you like.

This doesn't look quite right to me. I don't think we should treat the delete this; for a destructor as being emitted-for-device in any translation unit in which the vtable is marked used. (For example, if in your testcase MSEmitDeletingDtor::CFileStream::CFileStream() were a __host__ function, I think you'd still diagnose, but presumably shouldn't do so, because the vtable -- and therefore CFileStream::operator delete -- is never referenced / emitted for the device.) Instead, I think we should treat the delete this; as being emitted in any translation unit in which the vtable itself is emitted-for-device. Presumably, this means you will need to model / track usage of the vtable itself in your "call graph".

In D71714#1795313, @ilya wrote:

While I agree with the general notion about the flaws with the current implementation, I feel that the more major refactoring proposed in this review is out of the scope of this minor fix.

This seems reasonable to me, and matches GCC 10's behavior; $ is not in the basic source character set, so encoding it with a UCN should be permitted.

In D71714#1792085, @efriedma wrote:

To be rigorous, we should perform "pointer" checking for every operation that performs pointer arithmetic. Then we should perform "lvalue" checking (which doesn't allow pointers one past the end) in the following places:

1. When we take the address of an lvalue.
2. When we form a reference to an lvalue.
3. When we perform an lvalue-to-rvalue conversion.
4. When we perform an assignment to an lvalue.
   
   This sort of piecemeal approach of recursively looking through arbitrary expressions seems likely to miss cases. For example, we currently don't perform checks inside compound literals.

Summary of an off-line discussion with Ilya:

Seems fine to me. @aaron.ballman, want to take a look?

In addition to @riccibruno's comment, I found a couple of other suspicious things nearby (unrelated to the fix in this patch). No need to address them in this patch unless you feel motivated :)

Committed as rG4b0029995853fe37d1dc95ef96f46697c743fcad.