search

cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

Uniformize types in RPC method arguments #173

Closed lpetre-ulb closed 4 years ago

lpetre-ulb commented 4 years ago

Brief summary of issue

Currently, the RPC method types are completely different between different parts of the code. Most of the code base also passed by reference argument, even for integral types.

With https://github.com/cms-gem-daq-project/xhal/pull/147, I would recommend the following rules to pass arguments to RPC methods:

After discussion, using constant references (const &) for all RPC method parameters has been selected for increased standardization.

Types of issue

Expected Behavior

The RPC method types should be uniform throughout the code base.

Current Behavior

The RPC methods types are completely different between different parts of the code: arguments are passed by value or by reference, can be constant or not constant, ...

Context (for feature requests)

Clean up; provide semantically correct types for the RPC methods.

Your Environment

jsturdy commented 4 years ago

With cms-gem-daq-project/xhal#147, I would recommend the following rules to pass arguments to RPC methods:

  • All arguments should be constant: 'inout' and 'out' arguments are not supported in the templated RPC, it should be made clear;
  • The integral types should be passed by value and the non-integral types by reference. It enables the best performances for any kind of object.

It should also be made clear that in the declarations, if you're not passing the parameter by const &, you should not mention const at all, as it will be disregarded by the compiler (meaning that someone relying on the API can't really trust the const and that the compiler won't actually check that you haven't modified the parameter in the function block if you forgot to add const to the parameter in the definition signature), rather, simply ensure that const is in the definition such that the compiler can enforce const-correctness. As a general rule, if you want to require all parameters are const, it's probably a lot easier to just uniformly adopt const & declarations and definitions (simple for humans, simple for compilers), for a likely imperceptible performance difference

lpetre-ulb commented 4 years ago

With cms-gem-daq-project/xhal#147, I would recommend the following rules to pass arguments to RPC methods:

  • All arguments should be constant: 'inout' and 'out' arguments are not supported in the templated RPC, it should be made clear;
  • The integral types should be passed by value and the non-integral types by reference. It enables the best performances for any kind of object.

It should also be made clear that in the declarations, if you're not passing the parameter by const &, you should not mention const at all, as it will be disregarded by the compiler (meaning that someone relying on the API can't really trust the const and that the compiler won't actually check that you haven't modified the parameter in the function block if you forgot to add const to the parameter in the definition signature), rather, simply ensure that const is in the definition such that the compiler can enforce const-correctness.

Sure, you are right. In this case, the idea behind requiring the parameters to be const in the function declaration was to give a hint to the developer reading the API. While the RPC method calls are very similar to local function calls, they are not identical.

But, requiring the function declaration and definition to always be synchronized may be difficult to enforce...

As a general rule, if you want to require all parameters are const, it's probably a lot easier to just uniformly adopt const & declarations and definitions (simple for humans, simple for compilers), for a likely imperceptible performance difference

I'm not sure I like the idea of passing all parameters by constant reference... This is rarely (never?) done for regular functions.

mexanick commented 4 years ago

Since I'm working on a current templated RPC integration into cmsgemos, I have anyway to provide certain changes to the ctp7_modules w.r.t. export APIs. I can perform the uniformization on the way, but we need first to decide on what do we want.

I'm not sure I like the idea of passing all parameters by constant reference... This is rarely (never?) done for regular functions.

IMO it is not a bad idea to use the constant references everywhere for RPC calls. This will emphasize their distinct nature and provide uniformity for indeed an imperceptible performance drop as @jsturdy said.

lpetre-ulb commented 4 years ago

Since I'm working on a current templated RPC integration into cmsgemos, I have anyway to provide certain changes to the ctp7_modules w.r.t. export APIs. I can perform the uniformization on the way, but we need first to decide on what do we want.

Sure, but could you please do it in a separate PR, so it is easier to review. ;)

I'm not sure I like the idea of passing all parameters by constant reference... This is rarely (never?) done for regular functions.

IMO it is not a bad idea to use the constant references everywhere for RPC calls. This will emphasize their distinct nature and provide uniformity for indeed an imperceptible performance drop as @jsturdy said.

The performance drop wouldn't even exist for the remote calls. My only concern was that using constant reference for integral types is an usual practice which may confuse newcomers. Using non-constant parameters also allowed to discover the bug fixed in https://github.com/cms-gem-daq-project/xhal/pull/147.

Anyway, I can accept the standardization argument.

mexanick commented 4 years ago

well, a PR to just expose a couple of new ctp7_modules functions (which I need anyway) is small, just few lines, so addition in the same PR the standardization of APIs will certainly enlarge it, but won't make it difficult to read (and the new exports will be added with already standard arg passing). On the cmsgemos side the callers are anyway rewritten because we go from non-templated to templated framework, so there it would not increase amount of changes, the changes just would be somewhat different.

lpetre-ulb commented 4 years ago

There should be any impact on the cmsgemos, and, yes, this is clearly part of migration started in https://github.com/cms-gem-daq-project/cmsgemos/pull/327.

For the ctp7_modules, I don't see good reason to merge the two PR: one would be "Standardize the RPC methods argument parameters" while the other one would be "Export new function" (+ shuffle things around, which should technically be part of another commit ;). Why would a small PR be a bad PR? Reviewing only small silly changes is easier IMO. The second PR can be rebased on the first one.

mexanick commented 4 years ago

ok, #171 is ready for review then. Other changes will be added in subsequent PR

lpetre-ulb commented 4 years ago

Fixed in #178.