[PATCH] D122981: [Clang] CWG 1394: Incomplete types as parameters of deleted functions

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao added a comment.

Revisiting @ChuanqiXu 's code suggestion...



================
Comment at: clang/include/clang/Sema/Sema.h:2899-2909
+    /// C++ [dcl.fct.def.general]p1
+    /// function-body:
+    ///   = delete ;
+    ///   = default ;
+    Delete,
+    Default,
+
----------------
erichkeane wrote:
> rZhBoYao wrote:
> > ChuanqiXu wrote:
> > > rZhBoYao wrote:
> > > > ChuanqiXu wrote:
> > > > > Agree to @erichkeane 
> > > > With all due respect, this code suggestion doesn't make any sense to me. My best guess is @ChuanqiXu was thinking the order specified by the grammar as noted in [[ https://eel.is/c++draft/dcl.fct.def.general#nt:function-body | dcl.fct.def.general p1 ]]. Even if that was the case, `CompoundStmt` is not quite right either. Also, differentiating `ctor-initializer[opt] compound-statement` and `function-try-block` is meaningless here, hence the name `Other`.
> > > > 
> > > > I adopted the same order as to how `Parser::ParseFunctionDefinition` has always been parsing `function-body`. The order is not significant in any meaningful way as each of the 4 grammar productions of `function-body` is VERY different and mutually exclusive. Putting `Delete` and `Default` upfront not only emphasizes the "specialness" of them but also conveys how we handle `function-body`.
> > > > 
> > > > What say you, @erichkeane ?
> > > Yeah, the order comes from the standard. I think the comment should be consistent with the spec. And for the name, I agree `CompoundStmt` is not accurate indeed... I thought Default should be a good name but there is `Default` already. But I don't feel `Other` is good since the compound-statement is much more common.
> > > 
> > > > Putting Delete and Default upfront not only emphasizes the "specialness" of them but also conveys how we handle function-body.
> > > 
> > > This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.
> > > But I don't feel `Other` is good since the compound-statement is much more common.
> > `Other` reads "not special like `Delete` and `Default`".
> > > This don't makes sense. Generally, we would put common items in front. And the order here shouldn't be related to the implementation.
> > Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.
> > 
> > I'll await @erichkeane 's final verdict.
> >>Think of it as a not-so-special else clause at the end of an if/else chain. We tend to process special cases upfront. It is only natural.
> 
> This was my understanding.  This is a single value that means 'not default and not deleted'.  I think the idea of doing it in standards order is a really good one, but have the top 2 just both result in `Other`, `Unknown`, etc.  So something like:
> 
> ```
> //ctor-initializeropt compound-statement
> //function-try-block
> Unknown,
> //= default ;
> Default,
> //= delete ;
> Delete
> ```
> 
> BUT as this is not an 'ast' level flag, it is just for the purposes of simplifying this call, I don't think it needs to conform that closely.
I actually don't know what you were trying to say here:
>  We don't yet part function-try-block yet.
>  FunctionTryBlock,
especially:
> We don't yet part function-try-block yet.

Nonetheless, I did my best to cater to your comment within reason, such as reordering enumerators to align with the C++ standard text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122981