cms-gem-daq-project / xhal

XHAL interface library
0 stars 10 forks source link

Add more types to the templated RPC #123

Closed lpetre-ulb closed 5 years ago

lpetre-ulb commented 5 years ago

Description

This PR adds support for new types in the templated RPC methods:

Types of changes

Motivation and Context

This is the continuation of the the previous PR about the templated RPC methods.

How Has This Been Tested?

It has been tested with the help of a dummy CPT7 module (reply with and/or log the method parameter(s)) and a ~10 lines C++ program on the DAQ machine.

Checklist:

lpetre-ulb commented 5 years ago

Support for types defined in libraries added as well as one custom type serialization example.

lpetre-ulb commented 5 years ago

@lmoureaux's comments addressed.

lpetre-ulb commented 5 years ago

One minor comment that can be addressed in a future PR is the style for namespaces in header files.

I agree that we must use a uniform style; I tried to follow the cmsgemos CONTRIBUTING guideline, but it is not written there. Since one or two small PRs will be required to bring the last touch I also think it can be delayed until then.

I don't have a strong (enough to lay down the law) preference for either (though I will continue to use the latter in cmsgemos), but I do have a requirement for consistency, so if someone has a strong preference one way or the other (@mexanick, @lmoureaux, @lpetre-ulb, @bdorney), please raise it, otherwise, I will adopt the same convention as is used in our other code.

My only concern with the current style is that deeply nested namespaces increase the indentation by a lot, especially if one indentation is 4 spaces.

jsturdy commented 5 years ago

I don't have a strong (enough to lay down the law) preference for either (though I will continue to use the latter in cmsgemos), but I do have a requirement for consistency, so if someone has a strong preference one way or the other (@mexanick, @lmoureaux, @lpetre-ulb, @bdorney), please raise it, otherwise, I will adopt the same convention as is used in our other code.

My only concern with the current style is that deeply nested namespaces increase the indentation by a lot, especially if one indentation is 4 spaces.

There are those (I am not one) who mandate a style of not indenting namespaces..., for me indentation adds visual scope information, and as we live and code in the 21st century, I am not concerned with strict 80 character line limits (though the way github displays files with ample unused whitespace on either side of the main text, could be a reason to adopt some guideline...)

mexanick commented 5 years ago

I'd support the most populated namespace identation style aka

namespace xhal {
  namespace rpc {
/// content
  }
}