Make alias input_type protected for join_node

[dbiswas2808]

Currently the public aliases exposed by flow_graph::join_node includes input_type. This is a bit confusing since join_node has multiple input ports and I believe other nodes(with multiple inputs) don't have the same issue. I have been leveraging substitution failure with input_ports_type and input_type and similarly for output_ports_type and output_type for implementing a higher level interface for flow_graph. AFAIT only join_node breaks this distinction. Also, having join_node expose input_type as a public alias could be confusing to users.

[kboyarinov]

@dbiswas2808, could you please remove the line related to another PR from this patch and it would be approved and merged.
Thank you

[dbiswas2808]

@dbiswas2808, could you please remove the line related to another PR from this patch and it would be approved and merged.
Thank you

Done!

[kboyarinov]

LGTM

[dbiswas2808]

@kboyarinov

    template<typename JP, typename InputTuple, typename OutputTuple>
    class join_node_base : public graph_node, public join_node_FE<JP, InputTuple, OutputTuple>,
                           public sender<OutputTuple> {
    protected:
        using graph_node::my_graph;
    public:
        typedef OutputTuple output_type;

I think making the aliases protected might fix the failures. If you look at the above snippet the OutputTuple typedef uses the OutputType from join_node_FE. But that may make all the unwanted typedefs public again as the join_node_FE is publicly inherited.
How about we make declare these as
typedef OutputTuple OutputTuple;?

[kboyarinov]

@dbiswas2808, as I understand the testing failure, the problem is the following ( I have simplified a code a bit):

template <typename Tuple>
class join_node_FE {
    static const int N = std::tuple_size<Tuple>::value;
};

template <int N, typename Tuple>
class unfolded_join_node : public join_node_FE<Tuple> {
public:
    typedef typename wrap_tuple_elements<N, Tuple>::type input_ports_type; // problem is here
};

Visual Studio compiler for some reason interprets the usage of N on the last line as an access to private field of the base class, not the local non-type template parameter N.
Unfortunately, I failed to reproduce this on my local machine, I would try on other machines to figure out how this can be fixed

[dbiswas2808]

@dbiswas2808, as I understand the testing failure, the problem is the following ( I have simplified a code a bit):

template <typename Tuple>
class join_node_FE {
    static const int N = std::tuple_size<Tuple>::value;
};

template <int N, typename Tuple>
class unfolded_join_node : public join_node_FE<Tuple> {
public:
    typedef typename wrap_tuple_elements<N, Tuple>::type input_ports_type; // problem is here
};

Visual Studio compiler for some reason interprets the usage of N on the last line as an access to private field of the base class, not the local non-type template parameter N. Unfortunately, I failed to reproduce this on my local machine, I would try on other machines to figure out how this can be fixed

@kboyarinov That is strange indeed I haven't seen that myself before. Any updates on how to fix this or get around this issue?

[BenFrantzDale]

Should there be a comment here saying why these are private?

[kboyarinov]

As discussed above, this parameter is part of the internal implementation details of join_node_FE so it should not be part of oneapi::tbb::join_node public API.
Do you think we really need an extra comment in the code for this?

[BenFrantzDale]

🤷 I'm just trying to consider a future reader looking here without much context thinking "Why are N, output_type, input_type, and base_node_type private?" I'm thinking that would be clarified if they saw // These parameters are private because they are part of the internal implementation details of join_node_FE: After all, a join node doesn't have a single input port.

Just a thought.

[kboyarinov]

@dbiswas2808, as a simplest way to workaround a VS issue, I would propose to rename the template parameter of unfolded_join_node from N to something different. I guess it should fix the problem.

[dbiswas2808]

@dbiswas2808, as a simplest way to workaround a VS issue, I would propose to rename the template parameter of unfolded_join_node from N to something different. I guess it should fix the problem.

@kboyarinov I just Made the suggested change. Hopefully it compiles fine on MSVC.

[dbiswas2808]

@kboyarinov I've fixed the msvc compile issue as you suggested in the last commit. LMK if it looks right.

Failed CI

[kboyarinov]

@dbiswas2808, some checks failed - seems like N was not renamed in all required places. Please check this and then it can be merged.

[dbiswas2808]

@kboyarinov I fixed the issue I think. Seems to build on my MacOS system

[kboyarinov]

Thanks for the contribution! LGTM!

[kboyarinov]

@dbiswas2808, one last request from my side. Could you please add a sign-off string to the commit message? See Contribution.md for details. After that, the PR can be merged to master.
Thank you once again for your contribution.

[dbiswas2808]

@kboyarinov I just signed off all the commits and f-pushed it. Will need your approval for the pr-builder to start I think