Skip to content

Instantly share code, notes, and snippets.

@bgschiller
Last active April 27, 2020 17:42
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save bgschiller/4b983ec8adc03198ab9b86c483f68194 to your computer and use it in GitHub Desktop.
Save bgschiller/4b983ec8adc03198ab9b86c483f68194 to your computer and use it in GitHub Desktop.
FormalSignature problem
test
variadic
main

The main goal

FormalSignature represents the arguments for a function in an interpreted language. You can imagine we're writing the interpreter in C++, and want to implement a few built-in functions in C++ rather than in the interpreted language. To make that work safely, FormalSignature gives us a mapping from types in the interpreted language to types in C++.

Each parameter to the built-in is represented by a ParameterSpec, templated on the C++ type and containing some fields to represent the parameter names and the type in the embedded language.

A FormalSignature for Euclid's Greatest Common Divisor algorithm might look like this:

const FormalSignature<
  ParameterSpec<unsigned int>,
  ParameterSpec<unsigned int>
> Gcd{
  "GCD", "Compute the greatest common divisor of the first two arguments. Place the result in the third",
  ParameterSpec<unsigned int>{
    "a", // name of the first arg
    "positive integer", // typeId for the embedded language
    InputOrOutput::Input
  },
  ParameterSpec<unsigned int>{
    "b", "positive integer", InputOrOutput::Input
  },
  ParameterSpec<unsigned int>{
    "d", "positive integer", InputOrOutput::Output
  }
};

In order to invoke one of these built-ins, the FormalSignature must be bound to actual values that exist within the runtime language. We call this operation connect and pass in a vector<string> of the names (of variables in the interpreted language) that the function is being called with. This produces an ActualSignature. (We're using "formal" and "actual" parameters in the programming language sense. The formals are the names internal to the built-in. The actuals are the values on which the function is invoked.)

Problem one: spread tuple as arguments solved with invoke_hpp

(Solved this using https://github.com/BlackMATov/invoke.hpp)

We have a nice solution (thanks to Scott) for converting ParameterSpec<T>s into ActualParameter<T>s. It produces a std::tuple<ActualParameter<T>, ActualParameter<U>, ...> as we need. But it accepts a variable number of ParameterSpec<T> arguments. In a FormalSignature, I have them stored in a tuple.

How can I apply this tuple of arguments into the connect function. A minimal example removed from context is in variadic.cpp.

Problem two: type alias for the ActualSignature.

I'm okay with explicitly listing out the template parameters when instantiating a FormalSignature. However, I don't want to have to repeat that information when I create an ActualSignature using FormalSignature<Ts...>::connect, nor when I write a class that will own an ActualSignature. The signatures in the actual code frequently have 10-15 parameters, and my goal with this refactor is to avoid duplicating the knowledge of their names and order.

How can I expose a template alias, FormalSignature<Ts...>::ActualSignatureT that has all the template parameters filled in? For example:

using Example = FormalSignature<
  ParameterSpec<unsigned int>,
  ParameterSpec<string>,
  ParameterSpec<double>
>;

static_assert(std::is_same<
  Example::ActualSignatureT,
  ActualSignature<
    ActualParameter<unsigned int>,
    ActualParameter<string>,
    ActualParameter<double>
  >
>::value);
Possible solution

I feel like I'm close with this, but can't quite make it work:

// AsParam converts from a ParameterSpec<T> to an ActualParameter<T> (this part works)
template<typename T>
struct AsActualParam {
    using Param = T;
};

template <template <typename> class PSpec, typename T>
struct AsActualParam<PSpec<T>> {
    static_assert(std::is_same<PSpec<T>, ParameterSpec<T>>::value, "AsActualParam can only be used with a ParameterSpec<T>");
    using Param = ActualParameter<T>;
};
// succeeds
static_assert(std::is_same<
  AsActualParam<ParameterSpec<double>>::Param,
  ActualParameter<double>
>::value);

Then, we need to do the same thing for a parameter pack of ParamSpec...

template <
    class PSpec, // does this need to be template<typename> class PSpec?
    typename... Rest>
struct AsActualSignature {
    using ActualSignatureT = ActualSignature<
      // Pretty sure the beginning is right.
      AsParam<PSpec>::Param,

      // This line feels wrong, ActualSignatureT is a concrete type, not a parameter pack
      AsActualSignature<Rest...>::ActualSignatureT...
    >;
};
// also, feels like I need a base case for AsActualSignature?

Another potential solution

```cpp
template <typename... PSpecs>
struct AsActualSignature {
    using ActualSignatureT = ActualSignature<
        (AsParam<PSpecs>::Param)...
        // I want the line above to expand PSpecs to produce
        // AsParam<PSpecs0>::Param, AsParam<PSpecs1>::Param, AsParam<PSpecs2>::Param, ...
        // but it doesn't looks like that's a supported type of expansion
    >;
};

static_assert(std::is_same<
    ActualSignature<ActualParameter<double>, ActualParameter<int>>,
    AsActualSignature<ParameterSpec<double>, ParameterSpec<int>>::ActualSignatureT
>::value);

// This is the goal, but it currently fails. static_assert(std::is_same< AsActualSignature< ParameterSpec, ParameterSpec, ParameterSpec

::ActualSignatureT, ActualSignature< ActualParameter, ActualParameter, ActualParameter

::value);


I've been playing with this problem in [main.cpp](#file-main-cpp)
#include <tuple>
template<typename T>
struct Egg{
T contents;
};
template<typename T>
struct Chick{};
template <typename... Chicks>
struct LoudNest {
std::tuple<Chicks...> chicks;
};
template<typename... Eggs>
struct AsLoudNest {};
template<typename... Ts>
struct AsLoudNest<Egg<Ts>...> {
using type = LoudNest<Chick<Ts>...>;
};
template <typename... Eggs>
struct QuietNest {
std::tuple<Eggs...> eggs;
using LoudNestT = AsLoudNest<Eggs...>::type;
LoudNestT hatch() const {
auto hatchlings = std::apply(
[&](auto... args) { return std::make_tuple(hatchOne(args)...); },
eggs);
return LoudNestT{hatchlings};
}
template<typename T>
Chick<T> hatchOne(Egg<T> egg) {
return Chick<T>{};
}
};
const QuietNest<Egg<unsigned int>, Egg<unsigned short>, Egg<const char *>> theRobbins{
std::make_tuple(Egg<unsigned int>{12}, Egg<unsigned short>{14}, Egg<const char *>{"Thomas"})
};
auto theRobbinsInSpring = theRobbins.hatch();
static_assert(std::is_same<
decltype(theRobbinsInSpring),
LoudNest<Chick<unsigned int>, Chick<unsigned short>, Chick<const char *>>
>::value,
"Expected QuietNest<Eggs...>::hatch to produce a LoudNess<Chicks...> where each chick has a matching type parameter to the egg in the same position");
template<typename T>
struct AsChick {
using type = T;
};
template< template <typename> class E, typename T>
struct AsChick<E<T>> {
static_assert(std::is_same<E<T>, Egg<T>>::value, "Expected AsChick to be used with an Egg<T>");
using type = Chick<T>;
};
static_assert(std::is_same<
AsLoudNest<Egg<int>, Egg<double>>::type,
LoudNest<Chick<int>, Chick<double>>
>::value, "Expected AsLoudNest1 to convert my Egg<T>s to Chick<T>s");
/*******************************************************************************
* This file is part of the "https://github.com/blackmatov/invoke.hpp"
* For conditions of distribution and use, see copyright notice in LICENSE.md
* Copyright (C) 2018-2020, by Matvey Cherevko (blackmatov@gmail.com)
******************************************************************************/
#pragma once
#include <tuple>
#include <utility>
#include <functional>
#include <type_traits>
#define INVOKE_HPP_NOEXCEPT_RETURN(...) \
noexcept(noexcept(__VA_ARGS__)) { return __VA_ARGS__; }
#define INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(...) \
noexcept(noexcept(__VA_ARGS__)) -> decltype (__VA_ARGS__) { return __VA_ARGS__; }
//
// void_t
//
namespace invoke_hpp
{
namespace impl
{
template < typename... Args >
struct make_void {
using type = void;
};
}
template < typename... Args >
using void_t = typename impl::make_void<Args...>::type;
}
//
// is_reference_wrapper
//
namespace invoke_hpp
{
namespace impl
{
template < typename T >
struct is_reference_wrapper_impl
: std::false_type {};
template < typename U >
struct is_reference_wrapper_impl<std::reference_wrapper<U>>
: std::true_type {};
}
template < typename T >
struct is_reference_wrapper
: impl::is_reference_wrapper_impl<std::remove_cv_t<T>> {};
}
//
// invoke
//
namespace invoke_hpp
{
namespace impl
{
//
// invoke_member_object_impl
//
template
<
typename Base, typename F, typename Derived,
typename std::enable_if_t<std::is_base_of<Base, std::decay_t<Derived>>::value, int> = 0
>
constexpr auto invoke_member_object_impl(F Base::* f, Derived&& ref)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
std::forward<Derived>(ref).*f)
template
<
typename Base, typename F, typename RefWrap,
typename std::enable_if_t<is_reference_wrapper<std::decay_t<RefWrap>>::value, int> = 0
>
constexpr auto invoke_member_object_impl(F Base::* f, RefWrap&& ref)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
ref.get().*f)
template
<
typename Base, typename F, typename Pointer,
typename std::enable_if_t<
!std::is_base_of<Base, std::decay_t<Pointer>>::value &&
!is_reference_wrapper<std::decay_t<Pointer>>::value
, int> = 0
>
constexpr auto invoke_member_object_impl(F Base::* f, Pointer&& ptr)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
(*std::forward<Pointer>(ptr)).*f)
//
// invoke_member_function_impl
//
template
<
typename Base, typename F, typename Derived, typename... Args,
typename std::enable_if_t<std::is_base_of<Base, std::decay_t<Derived>>::value, int> = 0
>
constexpr auto invoke_member_function_impl(F Base::* f, Derived&& ref, Args&&... args)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
(std::forward<Derived>(ref).*f)(std::forward<Args>(args)...))
template
<
typename Base, typename F, typename RefWrap, typename... Args,
typename std::enable_if_t<is_reference_wrapper<std::decay_t<RefWrap>>::value, int> = 0
>
constexpr auto invoke_member_function_impl(F Base::* f, RefWrap&& ref, Args&&... args)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
(ref.get().*f)(std::forward<Args>(args)...))
template
<
typename Base, typename F, typename Pointer, typename... Args,
typename std::enable_if_t<
!std::is_base_of<Base, std::decay_t<Pointer>>::value &&
!is_reference_wrapper<std::decay_t<Pointer>>::value
, int> = 0
>
constexpr auto invoke_member_function_impl(F Base::* f, Pointer&& ptr, Args&&... args)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
((*std::forward<Pointer>(ptr)).*f)(std::forward<Args>(args)...))
}
template
<
typename F, typename... Args,
typename std::enable_if_t<!std::is_member_pointer<std::decay_t<F>>::value, int> = 0
>
constexpr auto invoke(F&& f, Args&&... args)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
std::forward<F>(f)(std::forward<Args>(args)...))
template
<
typename F, typename T,
typename std::enable_if_t<std::is_member_object_pointer<std::decay_t<F>>::value, int> = 0
>
constexpr auto invoke(F&& f, T&& t)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
impl::invoke_member_object_impl(std::forward<F>(f), std::forward<T>(t)))
template
<
typename F, typename... Args,
typename std::enable_if_t<std::is_member_function_pointer<std::decay_t<F>>::value, int> = 0
>
constexpr auto invoke(F&& f, Args&&... args)
INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN(
impl::invoke_member_function_impl(std::forward<F>(f), std::forward<Args>(args)...))
}
//
// invoke_result
//
namespace invoke_hpp
{
namespace impl
{
struct invoke_result_impl_tag {};
template < typename Void, typename F, typename... Args >
struct invoke_result_impl {};
template < typename F, typename... Args >
struct invoke_result_impl<void_t<invoke_result_impl_tag, decltype(invoke_hpp::invoke(std::declval<F>(), std::declval<Args>()...))>, F, Args...> {
using type = decltype(invoke_hpp::invoke(std::declval<F>(), std::declval<Args>()...));
};
}
template < typename F, typename... Args >
struct invoke_result
: impl::invoke_result_impl<void, F, Args...> {};
template < typename F, typename... Args >
using invoke_result_t = typename invoke_result<F, Args...>::type;
}
//
// is_invocable
//
namespace invoke_hpp
{
namespace impl
{
struct is_invocable_r_impl_tag {};
template < typename Void, typename R, typename F, typename... Args >
struct is_invocable_r_impl
: std::false_type {};
template < typename R, typename F, typename... Args >
struct is_invocable_r_impl<void_t<is_invocable_r_impl_tag, invoke_result_t<F, Args...>>, R, F, Args...>
: std::conditional_t<
std::is_void<R>::value,
std::true_type,
std::is_convertible<invoke_result_t<F, Args...>, R>> {};
}
template < typename R, typename F, typename... Args >
struct is_invocable_r
: impl::is_invocable_r_impl<void, R, F, Args...> {};
template < typename F, typename... Args >
using is_invocable = is_invocable_r<void, F, Args...>;
}
//
// apply
//
namespace invoke_hpp
{
namespace impl
{
template < typename F, typename Tuple, std::size_t... I >
constexpr decltype(auto) apply_impl(F&& f, Tuple&& args, std::index_sequence<I...>)
INVOKE_HPP_NOEXCEPT_RETURN(
invoke_hpp::invoke(
std::forward<F>(f),
std::get<I>(std::forward<Tuple>(args))...))
}
template < typename F, typename Tuple >
constexpr decltype(auto) apply(F&& f, Tuple&& args)
INVOKE_HPP_NOEXCEPT_RETURN(
impl::apply_impl(
std::forward<F>(f),
std::forward<Tuple>(args),
std::make_index_sequence<std::tuple_size<std::decay_t<Tuple>>::value>()))
}
#undef INVOKE_HPP_NOEXCEPT_RETURN
#undef INVOKE_HPP_NOEXCEPT_DECLTYPE_RETURN
#include <array>
#include <iostream>
#include <string>
#include <tuple>
#include <utility>
#include <vector>
#include "./invoke.hpp"
// A ParameterSpec
template<class T>
class ParameterSpec{};
template<typename T>
class ActualParameter {
public:
const std::string name;
const T value;
};
template<typename... Ps>
class ActualSignature {
public:
std::tuple<Ps...> params;
};
template <size_t... indices, typename Tuple>
auto tuple_subset(const Tuple& tpl, std::index_sequence<indices...>)
-> decltype(std::make_tuple(std::get<indices>(tpl)...))
{
return std::make_tuple(std::get<indices>(tpl)...);
// this means:
// make_tuple(get<indices[0]>(tpl), get<indices[1]>(tpl), ...)
}
template <typename Head, typename... Tail>
std::tuple<Tail...> tuple_tail(const std::tuple<Head, Tail...>& tpl)
{
return tuple_subset(tpl, std::index_sequence_for<Tail...>{});
// this means:
// tuple_subset<1, 2, 3, ..., sizeof...(Tail)-1>(tpl, ..)
}
// AsParam converts from a ParameterSpec<T> to an ActualParameter<T>
template<typename T>
struct AsParam {
using Param = T;
};
template <template <typename> class PSpec, typename T>
struct AsParam<PSpec<T>> {
static_assert(std::is_same<PSpec<T>, ParameterSpec<T>>::value, "AsParam can only be used with a ParameterSpec<T>");
using Param = ActualParameter<T>;
};
static_assert(
std::is_same<AsParam<ParameterSpec<double>>::Param, ActualParameter<double>>::value,
"Expected AsParam to convert a ParameterSpec<T> to an an ActualParameter<T>"
);
template <typename... PSpecs>
struct AsActualSignature {};
template <typename... Ts>
struct AsActualSignature<ParameterSpec<Ts>...> {
using type = ActualSignature<ActualParameter<Ts>...>;
};
static_assert(std::is_same<
ActualSignature<ActualParameter<double>, ActualParameter<int>>,
AsActualSignature<ParameterSpec<double>, ParameterSpec<int>>::type
>::value, "expected AsActualSignature to work");
template<typename... Ts>
class FormalSignature {
public:
std::string name;
std::tuple<Ts...> paramSpecs;
using ActualSignatureT = typename AsActualSignature<Ts...>::type;
ActualSignatureT connect(const std::vector<std::string>& args) const {
auto actualParams = invoke_hpp::apply(
[&](auto... ps) { return init(std::make_tuple(), args, 0, ps...); },
paramSpecs
);
return ActualSignatureT{actualParams};
}
template<typename... Actuals>
auto init (
std::tuple<Actuals...> actualParams,
const std::vector<std::string>& args,
const size_t& ix) const
{
return actualParams;
}
// paramSpecToActual recursion
template<typename... Actuals, typename T, typename... ParamSpecs>
auto init (
std::tuple<Actuals...> actualParams,
const std::vector<std::string>& args,
const size_t ix,
const ParameterSpec<T>& pSpec,
ParamSpecs... tail) const
{
ActualParameter<T> param = initOne<T>(pSpec, args[ix], ix);
return init (
std::tuple_cat (actualParams, std::make_tuple(param)),
args, ix + 1, tail...);
}
template<class T>
ActualParameter<T> initOne(const ParameterSpec<T>& spec, const std::string& arg, const size_t ix) const {
T value; // actually a lot more work here.
return ActualParameter<T>{arg, value};
}
};
const ParameterSpec<double> flowRate;
const FormalSignature<ParameterSpec<double>, ParameterSpec<double>> autoflowAlg{"AutoFlow", std::make_tuple(flowRate, flowRate)};
auto actualArgs = autoflowAlg.connect(std::vector<std::string>{"one", "two"});
static_assert(std::is_same<
decltype(actualArgs),
ActualSignature<ActualParameter<double>, ActualParameter<double>>
>::value, "Expected actualArgs to be ActualSignature");
int main() {
return 0;
}
main: main.cpp
g++ -o main -std=c++14 main.cpp
variadic: variadic.cpp
g++ -o variadic -std=c++14 variadic.cpp

Distribute template wrapper to parameter pack

I have a couple of templated types, Egg<T> and Chick<T>.

template<typename T>
struct Egg{};

template<typename T>
struct Chick{};

The chicks are contained in a class LoudNest<Chicks...> and the eggs in QuietNest<Eggs...>:

template <typename... Chicks>
struct LoudNest {
  std::tuple<Chicks...> chicks;
};

template <typename... Eggs>
struct QuietNest {
  std::tuple<Eggs...> eggs;

  // more here.
};

I want to have a hatch method on QuietNest<Eggs...> that produces a LoudNest. The LoudNest should have a Chick<T> for each Egg<T> in the QuietNest. I have a function QuietNest<Eggs...>::hatch_impl that can create a std::tuple<Chicks...> where the Chicks all have the correct type parameters. That is, QuietNest<Egg<double>, Egg<string>, Egg<char>>::hatch_impl will return std::tuple<Chick<double>, Chick<string>, Chick<char>>. I'm getting stuck trying to wrap that in a LoudNest constructor:

template <typename... Eggs>
struct QuietNest {
  std::tuple<Eggs...> eggs;

  auto hatch() const {
    // hatchlings is a std::tuple of chicks templated how I want.
    auto hatchlings = std::apply(
      [&](auto... args) { return hatch_impl(std::make_tuple(), args...); },
      eggs);
    // This line causes an error:
    return LoudNest{hatchlings};
    // error: cannot refer to class template 'LoudNest' without a template argument
  }

  // The rest of this all works, but is included in case you want to poke at it:

  // base case: only one parameter was passed—the tuple of hatched chicks.
  template<typename...Chicks>
  std::tuple<Chicks...> hatch_impl(std::tuple<Chicks...> chicks) {
    return chicks;
  }

  // recursive case: in addition to the tuple of hatched chicks,
  // at least one egg was passed (possibly more in the tail)
  template<typename...Chicks, typename T, typename...Unhatched>
  std::tuple<Chicks..., Chick<T>> hatch_impl(
    std::tuple<Chicks...> chicks,
    const Egg<T>& egg,
    Unhatched... tail
  ) const {
    Chick<T> babyBird = hatchOne(egg);
    return hatch_impl(
      std::tuple_cat(chicks, std::make_tuple(babyBird)),
      tail...);
  }

  template<T>
  Chick<T> hatchOne(Egg<T> egg) { return Chick<T>{}; }
};

I'm thinking I need to make a "converter" that accepts a parameter pack of eggs and produces a LoudNest with chicks of the corresponding types. Starting with converting a single Egg<T> to a Chick<T>, I have:

template<typename T>
struct AsChick {
  using type = T;
};

template< template <typename> class E, typename T>
struct AsChick<E<T>> {
  static_assert(std::is_same<E<T>, Egg<T>>::value, "Expected AsChick to be used with an Egg<T>");
  using type = Chick<T>;
};

Where I'm getting stuck is when I try to do the same for a parameter pack:

template<typename... Eggs>
struct AsLoudNest1 {
    using type = LoudNest<
        (AsChick<Eggs>::type)...
        // I want this to expand Eggs to produce
        // AsChick<Eggs0>::type, AsChick<Eggs1>::type, AsChick<Eggs2>::type, ...
        // but it doesn't looks like that's a supported type of expansion
    >;
};
static_assert(std::is_same<
  AsLoudNest1<Egg<int>, Egg<double>>::type,
  LoudNest<Chick<int>, Chick<double>>
>::value, "Expected AsLoudNest1 to convert my Egg<T>s to Chick<T>s");

And try number two:

template <
    class E, // does this need to be template<typename> class E?
    typename... Rest>
struct AsLoudNest2 {
    using type = LoudNest<
      // Pretty sure the beginning is right.
      AsChick<E>::type,

      // This line feels wrong, AsLoudNest2<...>::type is a concrete type, not a parameter pack
      AsLoudNest2<Rest...>::type...
    >;
};
// also, feels like I need a base case for AsLoudNest2?

My actual problem has to do with implementing an interpreter, and the classes are FormalParameter<T> (Egg<T>), ActualParameter<T> (Chick<T>), etc. However, I wanted to avoid using the word "parameter" for in the example code, as we're already talking about Parameter Packs in a different sense.

code from this post: https://godbolt.org/z/XBIEhm

#include <tuple>
#include <utility>
#include <iostream>
#include <cstddef>
#include <type_traits>
#include "./invoke.hpp"
template<typename F, typename Tuple, size_t ...S >
decltype(auto) apply_tuple_impl(F&& fn, Tuple&& t, std::index_sequence<S...>)
{
return std::forward<F>(fn)(std::get<S>(std::forward<Tuple>(t))...);
}
template<typename F, typename Tuple>
decltype(auto) apply_from_tuple(F&& fn, Tuple&& t)
{
std::size_t constexpr tSize
= std::tuple_size<typename std::remove_reference<Tuple>::type>::value;
return apply_tuple_impl(std::forward<F>(fn),
std::forward<Tuple>(t),
std::make_index_sequence<tSize>());
}
template <typename Arg, typename... Args>
void logAll(Arg&& arg, Args&&... args)
{
std::cout << std::forward<Arg>(arg);
// uhh, ignore this bit. It's not relevant to the problem.
// (at least, I don't think it is... I'll think more about it)
// I stole it from here: https://stackoverflow.com/a/27375675
using expander = int[];
(void)expander{0, (void(std::cout << ',' << std::forward<Args>(args)), 0)...};
std::cout << std::endl;
}
int main() {
// this works fine.
logAll(1,2,"asdf",4,"test");
// but I have a tuple of args, and want to pass them
// "spread" as params to addAll
auto tup = std::make_tuple(1,2,"testing",3);
// the internet tells me this should work:
invoke_hpp::apply(std::function<void(int,int, const char*, int)>(logAll<int, int, const char *, int>), tup);
// Specifying the template parameters for logAll won't be necessary,
// as we'll be calling `FormalSignature<Ts...>::connect`.
// (connect will be invoked from inside a fully-resolved FormalSignature)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment