Re: [PATCH v5 1/1] c++: Initial support for P0847R7 (Deducing This) [PR102609]

[Jason Merrill <jason@redhat.com>]

On 11/24/23 20:14, waffl3x wrote:
> OKAY, I figured out SOMETHING, I think this should be fine. As noted in
> the comments, this might be a better way of handling the static lambda
> case too. This is still a nasty hack so it should probably be done
> differently, but I question if making a whole new fntype node in
> tsubst_lambda_expr makes sense. On the other hand, maybe there will be
> problems if a lambda is already instantiated? I'm not sure, mutating
> things this way makes me uneasy though.

A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that 
all equivalent FUNCTION_TYPE share the same tree node (through 
type_hash_canon), so you're messing with the type of unrelated functions 
at the same time.  I think it's better to stick with the way static 
lambdas are handled.

> I don't like that pt.cc feels like it has a ton of hidden mutations,
> it's really hard to follow through it. Would you agree it's in need for
> cleanup or am I just not experienced enough in this area yet?

I'm sure there are things that could use cleaning up, but I'm not 
thinking of anything specific offhand.  Any particular examples?

> Regarding the error handling, I just had a thought about it, I have a
> hunch it definitely needs to go in tsubst_template_decl or
> tsubst_function_decl. There might need to be more changes to determine
> the actual type of the lambda in there, but everything else I've done
> changes the implicit object argument to be treated more like a regular
> argument, doing error handling for the object type in
> tsubst_lambda_expr would be inconsistent with that.

But the rule about unrelated type is specific to lambdas, so doing it in 
tsubst_lambda_expr seems preferable to adding a lambda special case to 
generic code.

> The other problem I'm having is
> 
> auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> This errors just fine, the lambda is unconditionally const so
> LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> 
> This on the other hand does not. The constness of the captures depends
> on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> here.
> auto f1 = [n = 5](this auto&& self){ n = 10; };
> as_const(f1)();

That sounds correct, why would this be an error?

The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P, 
it depends on the type of the object parameter, which in this case is 
non-const, so so are the captures.

> I was going to close out this message there but I just realized why
> exactly you think erroring in instantiate_body is too late, it's
> because at that point an error is an error, while if we error a little
> bit earlier during substitution it's not a hard error. I'm glad I
> realized this now, because I am much more confident in how to implement
> the errors for unrelated type now. I'm still a little confused on what
> the exact protocol for emitting errors at this stage is, as there
> aren't many explicit errors written in. It seems to me like the errors
> are supposed to be emitted when calling back into non-template stages
> of the compiler with substituted types. It also seems like that hasn't
> always been strictly followed, and I hate to contribute to code debt
> but I'm not sure how I would do it in this case, nor if I actually have
> a valid understanding of how this all works.

Most errors that could occur in template substitution are guarded by if 
(complain & tf_error) so that they aren't emitted during deduction 
substitution.  It's better if those are in code that's shared with the 
non-template case, but sometimes that isn't feasible.

> One side note before I close up here, I don't like when *_P macros are
> used to set flags. Is this something else we should clean up in the future?

I don't think so, a wholesale renaming would just confuse existing 
developers.

> I'm beginning to
> wonder if an overhaul that gets rid of the macros from the public
> interface is a good idea. I'm reluctant to suggest that as I've really
> warmed up to the macros a lot. They are used in a consistent and easy
> to understand way which is highly unlike the bad uses of macros that
> I've seen before that really obscure what's actually going on. But they
> are still macros, so maybe moving away from them is the right call,
> especially since there has started to be a mix-up of macros and
> functions for the same purposes. I'm mildly of the opinion that only
> one style should be used (in the public interface) because mixing them
> causes confusion, it did for me anyway. Perhaps I should open a thread
> on the general mail list and see what others think, get some input
> before I decide which direction to go with it. To be clear, when I say
> getting rid of macros, I want to emphasize I mean only in the public
> interface, I don't see any value in getting rid of macros under the
> hood as the way the checking macros are implemented is already really
> good and works. It would only cause problems to try to move away from
> that. I think I'll probably start to mess around with this idea as soon
> as this patch is done.

There has been some movement from macros toward inline functions since 
the switch to C++, but that also has complications with 
const-correctness, declaration order, and bit-fields.  It's probably 
good to use inlines instead of larger macros when feasible, but the 
accessors should probably stay macros.

Jason