- User Since
- Jul 12 2012, 2:19 PM (393 w, 5 d)
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?
Sun, Jan 26
Fri, Jan 24
This seems reasonable to me.
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.
Thu, Jan 23
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.
Wed, Jan 22
Tue, Jan 21
Mechanically, this looks fine.
Sun, Jan 19
Fri, Jan 17
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
Thu, Jan 16
Wed, Jan 15
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.
Tue, Jan 14
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!
Mon, Jan 13
(Partial comments; I'll try to complete the review today or tomorrow.)
Sun, Jan 12
Fri, Jan 10
Thu, Jan 9
Wed, Jan 8
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".
Tue, Jan 7
Mon, Jan 6
Dec 20 2019
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.
Dec 19 2019
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 :)
Dec 18 2019
Dec 16 2019
Committed as rG4b0029995853fe37d1dc95ef96f46697c743fcad.