[PATCH] D60934: [clang] adding explicit(bool) from c++2a

Gauthier via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Apr 26 09:38:03 PDT 2019


Tyker marked 2 inline comments as done.
Tyker added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+
----------------
rsmith wrote:
> Generally we don't want to have setters in the AST; the AST is intended to be immutable after creation. Is this necessary?
this is used in 2 cases:
- for deserialization.
- for in `Sema::ActOnFunctionDeclarator` to make every declaration of the same function have the same explicit specifier as the canonical one. there is probably a better way to do this but i didn't find how.

the second use case will need to be changed because the constructor's explicit specifier will be tail-allocated so the explicit specifier from the canonical declaration will need to be recovered before construction to allocate storage. 

how can we find the canonical declaration of a function before it is constructed ?


================
Comment at: clang/lib/Sema/SemaInit.cpp:3819
+        AllowExplicit = AllowExplicit && !CopyInitializing;
+        if (AllowExplicit || Conv->isMaybeNotExplicit()) {
           if (ConvTemplate)
----------------
rsmith wrote:
> Consider just removing this `if`. It's not clear that it carries its weight.
this if prevent conversion that are already known not to be valid from being added to the overload set. removing it is still correct because it will be removed later. but we would be doing "useless" work because we are adding the to the overload set knowing they will be removed.
the only benefit i see of removing this if is to make all explicit conversion appear in overload resolution failure messages. is it the intent ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60934/new/

https://reviews.llvm.org/D60934





More information about the cfe-commits mailing list