Make match optional inside examine

[sellout]

This doesn’t necessitate any changes to examine call sites, but the match
wrapper around the lambdas can now be elided.

This is useful down the road if we use clang-format or something else where
specifying custom pretty-printing for individual functions is difficult.

What now looks like

examine(meh, match {
    [](const Foo& foo) { … },
    [](const Bar& bar) { … },
});

would, under clang-format, likely end up similar to

examine(
    meh,
    match {
        [](const Foo& foo) { … },
        [](const Bar& bar) { … },
    });

(which takes two extra lines, plus an additional indentation) but now can
instead be

examine(
    meh,
    [](const Foo& foo) { … },
    [](const Bar& bar) { … });

which fits in the same space, with the same indentation as our current approach.

[sellout]

Review guidance: split view

[daira]

Suggested change

	constexpr decltype(auto) examine(Specimen&& specimen, Visitors&&... visitors) {
	decltype(auto) examine(Specimen&& specimen, Visitors&&... visitors) {

This function doesn't meet the requirements for constexpr functions, specifically that "its return value (if any) and each of its parameters must be of a LiteralType".

(std::visit also doesn't meet that requirement and is constexpr, but maybe it's magic.)

This is UB when reading the spec strictly, even though a good compiler is likely to either just ignore the constexpr or do the right thing (which would be to make it constexpr only if it can be).

[daira]

Oh, I missed this part:

For constexpr function templates and constexpr member functions of class templates, at least one specialization must satisfy the abovementioned requirements. Other specializations are still considered as constexpr, even though a call to such a function cannot appear in a constant expression. If no specialization of the template would satisfy the requirements for a constexpr function when considered as a non-template function, the template is ill-formed, no diagnostic required. (until C++23)

It is unclear whether the last sentence means "If no possible specialization" (which would be okay as-is), or "If no actually instantiated specialization" — in which case we'd have to include an example like this:

static inline void examine_can_be_constexpr() {
    static_assert(42 == examine(42, [](int x) constexpr { return x; }));
}

It makes very little sense that you would need to do that, but this is C++, ¯\_(ツ)_/¯.

[daira]

@str4d, @nuttycom and I talked this through and convinced ourselves that:

• the intended interpretation is probably "If no possible specialization";
• but, it is a good idea to include the above example function anyway, since it acts as a static test that the constexpr usage works.

[daira]

@str4d also pointed out that match would be a better name for examine as of this PR. match would then be renamed to something else, say matcher. (Outside this file, currently there is one nontrivial "bare" use of match at wallet/rpcwallet.cpp line 585, and one other at wallet/wallet_tx_builder.cpp line 35 that is replaced in this PR.)

[sellout]

overloaded is the name used in the example we copied it from. I don’t think it’s a good name, but I think removing the only remaining explicit match from rpcwallet.cpp should be easy, so then the name is pretty internal.

[daira]

If you look at the example in rpcwallet.cpp:

zcash/src/wallet/rpcwallet.cpp

Lines 585 to 595 in b072a89

	auto GetSourceForDestination = match {
	[&](const CKeyID& addr) -> std::optional<PaymentAddressSource> {
	return GetSourceForPaymentAddress(pwalletMain)(addr);
	},
	[&](const CScriptID& addr) -> std::optional<PaymentAddressSource> {
	return GetSourceForPaymentAddress(pwalletMain)(addr);
	},
	[&](const CNoDestination& addr) -> std::optional<PaymentAddressSource> {
	return std::nullopt;
	},
	};

the name matcher makes a lot of sense (more than overloaded). It's basically just treating the set of match clauses as a first-class object. And I don't think we need to discourage this construct from being used directly; it's sometimes the more natural approach to avoid code duplication.

[daira]

Suggested change

	examine(
	payment.address,
	examine(payment.address,

[daira]

Suggested change

	[&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; });
	[&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; }
	);

and similarly for the other uses.

[sellout]

It sounds like this was discussed in Ancient Issues today, so maybe we’re all on the same page already, but I wanted to check before I made all the changes. So, here’s a bit of summary, mostly for @nuttycom, @ebfull, and @str4d to respond to (and for @daira to call me out if I misrepresent something):

First, we can either get clang-format to approximate what we want, and get autoformatting, or add match (née examine) to WhitespaceSensitiveMacros (WSM) which effectively treats every call as if it were wrapped in clang-format off.

There are other *Macros lists (like ForEachMacros) that are less all-or-nothing than WSM, but I don’t think we can massage match into any of them.

Personally, I’m not that keen on WSM, especially for matches that don’t consist of 1-lineish lambdas – I like things being formatted for me. But, I’d much rather give up those sections of code formatting than lose them all. I also don’t really like the ) being treated like a brace in this one case, especially without the trailing ,. So, my own preferences are

1. let clang-format do what it wants

   match(
       meh,
       [](const Foo& foo) { … },
       [](const Bar& bar) { … });

2. figure out how to make a block macro for this (which I don’t think can work with the structure of this operation and CPP, but I’m happy to be proven wrong), like

   match(meh) {
       [](const Foo& foo) { … },
       [](const Bar& bar) { … },
   }

3. manually do what we want here, using WSM

   match(meh,
       [](const Foo& foo) { … },
       [](const Bar& bar) { … }
   );

But, none of that is very strong. I just want some consensus before I make the edits.

[str4d]

I am personally fine with 1, because 3 doesn't avoid the need to edit the last match alternative if we add another at the bottom. Also, reformatting later from any of these to the others won't touch any of the internal non-last match alternatives (except for moving away from 3 where maybe some internal whitespace changes), so I don't see that blocking later changes if we e.g. figure out how to do 2.

[daira]

Please either remove the constexpr or explain why it's ok.

Please add the suggested examine_can_constexpr function, and consider whether to do the suggested renaming of examine to match and match to matcher (or some other name).

[daira]

Suggested change

	// The C++ spec says that for a template function to be declared `constexpr`,
	// it must be possible for "at least one specialization" to satisfy the
	// `constexpr` requirements (which are documented at
	// https://en.cppreference.com/w/cpp/language/constexpr#constexpr_function ).
	// This example proves that this is possible.
	static inline void examine_can_be_constexpr() {
	static_assert(42 == examine(42, [](int x) constexpr { return x; }),
	"constexpr examine should work");
	}

[daira]

Consider also updating .clang-format to include examine (or match if we make that change) as a "whitespace-sensitive macro".

[sellout]

Consider also updating .clang-format to include examine (or match if we make that change) as a "whitespace-sensitive macro".

This is in #6678 – the current .clang-format is not useful for us, so we shouldn’t worry about updating it before something like #6678 is there. I only discovered it existed when I started using LSP again and my editor kept applying the incorrect .clang-format to my code as I wrote it.