Giter Site home page Giter Site logo

Comments (9)

HDembinski avatar HDembinski commented on June 6, 2024 1

Thanks for listing all that. I agree with you in general, but most of these caveats do not seem to matter for Boost.Histogram. In practice, it does not seem to be an issue.

  • We must use our own axis::variant instead of std::variant anyway.
  • You shouldn't use std::is_same with axis types. We have axis traits to write code which works with any axis.
  • std::common_type applied to axis types makes no sense.
  • We don't use std::type_info in this way and the library does not use type erasure for axis types.

etc.

That being said, I am not against supporting <axis>_t in addition to <axis> on C++17 and higher. This can be hidden behind an #if

from histogram.

HDembinski avatar HDembinski commented on June 6, 2024

@HDembinski Here are some examples why using variable_t can bring convenience.

@jhcarl0814
Ok, thanks, now I got it. If we can make this change without breaking the API, then I am all for it. In other words, I do not want to introduce variable_t, but use variable as the template alias and call the underlying struct variable_t. If that's not breaking any existing code.

from histogram.

jhcarl0814 avatar jhcarl0814 commented on June 6, 2024

@HDembinski Sorry I have to mention https://www.hyrumslaw.com/ again. We need to know how many users are relying on variable's type template parameters staying in a deduced context. If few people are using this, then it's safe to start the transition to the new approach.

e.g.

Some old code that are relying on variable's type template parameters staying in a deduced context:

template<typename T = int>
struct variable
{
    std::conditional_t<std::is_same_v<T, void>, int, T> data;
};

template<typename T>
void f1(variable<T>)
{
}
void f2()
{
    f1(variable<int>());
}

When replace struct variable with struct variable_t+using variable, template deduction stops working:

template<typename T = int>
struct variable_t
{
    T data;
};
template<typename T = int>
using variable = variable_t<std::conditional_t<std::is_same_v<T, void>, int, T>>;

template<typename T>
void f1(variable<T>) // Candidate template ignored: couldn't infer template argument 'T'
{
}
void f2()
{
    f1(variable<int>()); // No matching function cal to 'f1'
}

So require every user to change code to:

template<typename T = int>
struct variable_t
{
    T data;
};
template<typename T = int>
using variable = variable_t<std::conditional_t<std::is_same_v<T, void>, int, T>>;

template<typename T>
void f1(variable_t<T>) ///////////////////////////////////////
{
}
void f2()
{
    f1(variable<int>());
}

All other non-generic code (e.g. void f1(variable_t<>)) and generic code that don't care what's inside variable's template parameter list (e.g. void f1(T)) are not affected.

from histogram.

jhcarl0814 avatar jhcarl0814 commented on June 6, 2024

@HDembinski The type alias should work with template deduction guides, it's just not working now.
According to https://en.cppreference.com/w/cpp/language/class_template_argument_deduction#Deduction_for_alias_templates ,

  • for each constructor of variable:
    • the implicitly-generated deduction guides for variable_t is like
template<
    /*only `variable_t`'s definition's template parameters that appear in the deduction result*/,
    /*the constructor's template parameters that are not deduced from template arguments given in `variable_t`'s definition*/>
variable(/*the constructor's function parameters (the constructor's template parameters that are deduced from template arguments given in `variable_t`'s definition are replaced with the deduction result)*/)
->
variable<
    std::conditional_t<std::is_same_v<Value, use_default>, double, Value>,
    std::conditional_t<std::is_same_v<MetaData, use_default>, std::string, MetaData>,
    std::conditional_t<std::is_same_v<Options, use_default>, decltype(option::underflow | option::overflow), Options>,
    std::conditional_t<std::is_same_v<Allocator, use_default>, std::allocator<std::conditional_t< std::is_same_v<Value, use_default>, double, Value>>, Allocator>>;

// e.g.

template</*variable_t's*/class Value, /*variable_t's*/class MetaData, /*variable_t's*/class Options, /*variable_t's*/class Allocator, /*variable's*/class U>
variable(
    std::initializer_list<U>,
    typename boost::histogram::axis::metadata_base<
        std::conditional_t<
            std::is_same_v<std::conditional_t<std::is_same_v<MetaData, use_default>, std::string, MetaData>, boost::use_default>,
            std::string,
            std::conditional_t<std::is_same_v<MetaData, use_default>, std::string, MetaData>>,
        (std::is_empty_v<std::conditional_t</* ... */>> && std::is_final_v<std::conditional_t</* ... */>>)
    >::metadata_type,
    std::conditional_t<
        std::is_same_v<std::conditional_t<std::is_same_v<Options, use_default>, decltype(option::underflow | option::overflow), Options>, boost::use_default>,
        boost::histogram::axis::option::bitset<3>,
        std::conditional_t<std::is_same_v<Options, use_default>, decltype(option::underflow | option::overflow), Options>>,
    std::conditional_t<std::is_same_v<Allocator, use_default>, std::allocator<std::conditional_t< std::is_same_v<Value, use_default>, double, Value>>, Allocator>>
)->
variable<
    std::conditional_t<std::is_same_v<Value, use_default>, double, Value>,
    std::conditional_t<std::is_same_v<MetaData, use_default>, std::string, MetaData>,
    std::conditional_t<std::is_same_v<Options, use_default>, decltype(option::underflow | option::overflow), Options>,
    std::conditional_t<std::is_same_v<Allocator, use_default>, std::allocator<std::conditional_t< std::is_same_v<Value, use_default>, double, Value>>, Allocator>
>;

// U is in a deduced context and is deduced to float/double/long double etc,
// Value is in a non-deduced context and uses default template argument (double) in the end
  • for each user-defined deduction guide of variable: (I don't know why the compiler ignores all user-defined deduction guides of variable, looks like a compiler bug)
    • the user-defined deduction guides for variable_t is like
template<
    /*only `variable_t`'s definition's template parameters that appear in the deduction result*/,
    /*the deduction guide's template parameters that are not deduced from template arguments given in `variable_t`'s definition*/>
variable(/*the deduction guide's function parameters (the deduction guide's template parameters that are deduced from template arguments given in `variable_t`'s definition are replaced with the deduction result)*/)
->
variable</*the deduction guide's template arguments (the deduction guide's template parameters that are deduced from template arguments given in `variable_t`'s definition are replaced with the deduction result)*/>;

// e.g.

template </* none of variable_t's template parameters appear in the deduction result */ /*variable's*/class T>
variable(std::initializer_list<T>) -> variable<detail::convert_integer<T, double>, null_type>;

// T is in a deduced context and is deduced to float/double/long double etc,
// so we get variable<float/double/long double, null_type>

variable_t's type template parameters are in non-deduced context in compiler generated deduction guides (I don't know why the compiler only processes implicitly-generated deduction guides of variable and ignores all user-defined deduction guides of variable), as a result variable_t's type template parameters' default arguments are always used:

#include <boost/histogram/axis.hpp>
#include <boost/type_index.hpp>
#include<iostream>

namespace boost {
namespace histogram {
namespace axis {
    template<class Value___ = double, class MetaData___ = use_default, class Options___ = use_default, class Allocator___ = std::allocator<Value___>>
    using variable_t = boost::histogram::axis::variable<
        detail::replace_default<Value___, double>,
        detail::replace_default<MetaData___, std::string>,
        detail::replace_default<Options___, decltype(option::underflow |
        option::overflow)>, detail::replace_default<Allocator___,
        std::allocator<detail::replace_default<Value___, double>>>>;
} // namespace axis
} // namespace histogram
} // namespace boost

static_assert(__cpp_deduction_guides >= 201907L);

int main()
{
    {
        boost::histogram::axis::variable<> a1; std::cout<<boost::typeindex::type_id_with_cvr<decltype(a1)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t<> b1; std::cout<<boost::typeindex::type_id_with_cvr<decltype(b1)>().pretty_name()<<'\n'; std::cout<<'\n';

        boost::histogram::axis::variable a2; std::cout<<boost::typeindex::type_id_with_cvr<decltype(a2)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t b2;  std::cout<<boost::typeindex::type_id_with_cvr<decltype(b2)>().pretty_name()<<'\n'; std::cout<<'\n';

        boost::histogram::axis::variable a3({1,2,3}); std::cout<<boost::typeindex::type_id_with_cvr<decltype(a3)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t b3({1,2,3});  std::cout<<boost::typeindex::type_id_with_cvr<decltype(b3)>().pretty_name()<<'\n'; std::cout<<'\n';

        boost::histogram::axis::variable a4({1.0f,2.0f,3.0f}); std::cout<<boost::typeindex::type_id_with_cvr<decltype(a4)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t b4({1.0f,2.0f,3.0f});  std::cout<<boost::typeindex::type_id_with_cvr<decltype(b4)>().pretty_name()<<'\n'; std::cout<<'\n';

        boost::histogram::axis::variable a5({1.0,2.0,3.0}); std::cout<<boost::typeindex::type_id_with_cvr<decltype(a5)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t b5({1.0,2.0,3.0});  std::cout<<boost::typeindex::type_id_with_cvr<decltype(b5)>().pretty_name()<<'\n'; std::cout<<'\n';

        boost::histogram::axis::variable a6({1.0l,2.0l,3.0l}); std::cout<<boost::typeindex::type_id_with_cvr<decltype(a6)>().pretty_name()<<'\n';
        boost::histogram::axis::variable_t b6({1.0l,2.0l,3.0l});  std::cout<<boost::typeindex::type_id_with_cvr<decltype(b6)>().pretty_name()<<'\n'; std::cout<<'\n';
    }
}

PS:
Boost.Parameter allows omiting argument in the middle (by requiring users to specify parameters' names), but also does not address the issue: users also get different types by passing different template argument list, so it only verifies that "member types are correct" instead of "type template parameters are correct" at the end of the class_ example. (https://www.boost.org/doc/libs/release/libs/parameter/doc/html/index.html#named-template-parameters )

from histogram.

HDembinski avatar HDembinski commented on June 6, 2024

@HDembinski Sorry I have to mention https://www.hyrumslaw.com/ again. We need to know how many users are relying on variable's type template parameters staying in a deduced context. If few people are using this, then it's safe to start the transition to the new approach.

I am well aware of Hyrum's law, that's why I stated my preference so carefully. Okay, so replacing the struct name with an alias name does not work. :(

Do you think it is still useful to introduce variable_t in addition to variable? And likewise for every axis?

from histogram.

HDembinski avatar HDembinski commented on June 6, 2024

(I don't know why the compiler only processes implicitly-generated deduction guides of variable and ignores all user-defined deduction guides of variable)

That seems to be an issue with gcc only, the other compilers use the deduction guides.

I implemented some workarounds just recently here #373 to fix gcc>=11.

from histogram.

HDembinski avatar HDembinski commented on June 6, 2024

In my opinion, the current version is better than the variable_t one, the type is much more readable.

boost::histogram::axis::variable<double, boost::use_default, boost::use_default, std::allocator<double> >
boost::histogram::axis::variable<double, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, boost::histogram::axis::option::bitset<3u>, std::allocator<double> >

You should not make type checks like this anyway (what is the use case for such type checks)?

std::is_same_v<
    boost::histogram::axis::variable<double, std::string, boost::use_default>,
    boost::histogram::axis::variable<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)>
>

If you want to know what the options are, call options(), etc.

from histogram.

HDembinski avatar HDembinski commented on June 6, 2024

The main point you have in favor is:

This causes a compile error:

boost::histogram::axis::variable<double, std::string, boost::use_default> a;
boost::histogram::axis::variable<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)> b;
a = b;

This does not cause a compile error:

boost::histogram::axis::variable_t<double, std::string, boost::use_default> a;
boost::histogram::axis::variable_t<double, std::string, decltype(boost::histogram::axis::option::underflow | boost::histogram::axis::option::overflow)> b;
a = b;

This could be fixed with a converting ctor.

from histogram.

jhcarl0814 avatar jhcarl0814 commented on June 6, 2024

@HDembinski You are right. It's not transparent to switch to the type alias (and I forgot there's no CTAD for template type alias during [C++17, C++20)). So I'd better only use it in my own projects when need it.


The tricky part is, there are already so many places scattered around various libraries (e.g. standard library) that uses the "same type" type trait/concept that:

You only know there is a landmine when you step on it.

searching "same type" can give us an impression of how many places depend on the "same type" type trait/concept in standard library (maybe the doc should add a note to warn users so they will not shoot themselves in the foot?):

  • (operator=, could be fixed with a converting constructor)
  • (std::is_same and std::same_as gives unexpected result, other things might depend on them)
  • (std::is_base_of gives unexpected result, other things might depend on them)
  • (std::common_type gives unexpected result, other things might depend on them)
  • std::variant, users can not get/visit using t<boost::use_default> if it stores t<actual_type>, can not get/visit using t<actual_type> if it stores t<boost::use_default>
  • std::type_info::operator==, std::type_info::hash_code and std::any, when you use these kind of things it probably means you are passing type-erasured objects across lib boundaries, the receivers can not any_cast using ... if ..., can not any_cast using ... if ...
  • reinterpret_cast, also often used in type erasure, informally, two types are similar if, ignoring top-level cv-qualification: they are the same type; or ...
  • typedef, once declared, a typedef-name may only be redeclared to refer to the same type again
  • implicit conversions, the pointer and pointer-to-member conversions stops working and can't be fixed because pointers require that a non-temporary object actually exists (can't point to converting constructor generated temporary)
  • implicit conversions, you can only implicit convert once (two or more times are not supported), "using a converting constructor" uses up a turn for this
  • function declaration, if there are multiple return statements, they must all deduce to the same type
  • std::is_layout_compatible, similar types are not layout-compatible if they are not the same type after ignoring top-level cv-qualification

from histogram.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.