Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server_init_hook() method (#498) #551

Open
wants to merge 7 commits into
base: tango-9-lts
from

Conversation

3 participants
@Ingvord
Copy link
Member

commented May 13, 2019

@bourtemb

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@Ingvord, FYI, I just updated .travis/debian7/Dockerfile in tango-9-lts branch to fix the problems with Debian 7 Travis CI build.

@Ingvord

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@bourtemb, thanks! That would probably fix Travis in this branch too

@Ingvord Ingvord closed this May 14, 2019

@Ingvord Ingvord force-pushed the issue-498 branch from cc37373 to ab05abc May 14, 2019

@Ingvord Ingvord added this to Done in Tango 9 Long Term Support via automation May 14, 2019

@Ingvord Ingvord reopened this May 14, 2019

@Ingvord Ingvord requested a review from bourtemb May 15, 2019

@Ingvord

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@bourtemb , I think this is more or less done.

A few notes:

  • I pass DServer const reference into server_init_hook - maybe it will be useful
  • init_device call has been removed from DServer ctr - I prefer when there is no side effects
  • I am not sure about DevFailed handling in DServer::server_init_hook - maybe we have some utility function for such case, i.e. set device status using DevFailed
  • I use auto and since Travis does not complain even in Debian 7, I guess it is OK. But final call is up to you of course

@t-b, @mliszcz , could you please also review this PR? I do not see you in the reviwers list, so can not add you as reviewers. Thanks!

@bourtemb bourtemb requested a review from t-b May 15, 2019

@bourtemb
Copy link
Member

left a comment

@bourtemb , I think this is more or less done.

Thanks for the work done.

A few notes:

* I pass DServer const reference into server_init_hook - maybe it will be useful

I am not sure this is really useful. Users can do the following to get a reference to it if needed:

DServer * dserver_instance = Tango::Util::instance()->get_dserver_device();

* init_device call has been removed from DServer ctr - I prefer when there is no side effects

Does not seem stupid at first sight.

* I am not sure about DevFailed handling in DServer::server_init_hook - maybe we have some utility function for such case, i.e. set device status using DevFailed

I don't think we have anything like that for the moment... But I might be wrong.
I think it would be useful to have an std::ostream& Tango::operator<<(std::ostream&, const Tango::DevFailed&) method to be able to create string streams easily from a DevFailed exception.

For the moment, we only have Tango::Except::print_exception(const Exception &) which is able to print to stderr a Tango Exception.

* I use **auto** and since Travis does not complain even in Debian 7, I guess it is OK. But final call is up to you of course

We will get into trouble with appveyor.

@t-b, @mliszcz , could you please also review this PR? I do not see you in the reviwers list, so can not add you as reviewers. Thanks!

I invited @t-b and @mliszcz to be part of the cppTango developers team so once Michal will have accepted, you will be able to add him as reviewer as I did with @t-b.

I would also invite @reszelaz to review this PR since he was the one triggering the development of this new feature by creating tango-controls/TangoTickets#7.

* <b>DevFailed</b> exception specification
*/
virtual void server_init_hook(const DServer &)
{};

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 15, 2019

Member

This Pull Request will result in a new major version of Tango because we lose the binary compatibility by adding this virtual function. This is not a problem but we should keep this in mind when updating the version number.
By the way, I created this wiki page which refers to a web page explaining the Do's and Don'ts to preserve binary compatibility.

@@ -171,7 +173,6 @@ protected :
bool is_event_name(string &);
bool is_ip_address(string &);

bool from_constructor;

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 15, 2019

Member

from_constructor does not seem to be used any longer indeed.

@@ -1845,12 +1845,15 @@ void DServerClass::device_factory(const Tango::DevVarStringArray *devlist_ptr)
//
// Create device and add it into the device list
//
auto dserver = new DServer(this,

This comment has been minimized.

Copy link
@bourtemb

bourtemb May 15, 2019

Member

We should not use auto if we want to be able to compile on old windows compilers.

Ingvord added some commits May 16, 2019

@mliszcz
Copy link
Collaborator

left a comment

OK for me, just a few comments.

@@ -217,6 +217,11 @@ void DevTest::always_executed_hook()

}

void DevTest::server_init_hook()
{
cout2 << "In server_init_hook method" << endl;

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 20, 2019

Collaborator

Consider using std::endl, otherwise it may break with #528 (general comment, applies to other places as well)

This comment has been minimized.

Copy link
@Ingvord

Ingvord May 22, 2019

Author Member

This is a DevTest - a part of cpp_test_suite, so I think it is not relevant here to use std:: as anyway in other parts of this class it is missing. I will add std:: only for this line as it is a new one and should not go in conflict when #528 is merged and then fix the issues if any.

{
for (int i = 0, size = devFailed.errors.length(); i < size; ++i)
{
DevError error = devFailed.errors[i];

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 20, 2019

Collaborator

Consider using reference instead of a copy. DevError has CORBA string members which allocate and copy memory from their copy-ctors.

This comment has been minimized.

Copy link
@Ingvord

Ingvord May 22, 2019

Author Member

ok.

ss << "\tOrigin:" << error.origin << "\n";
ss << "\tSeverity:" << error.severity << "\n";
ss << "\tReason:" << error.reason << "\n";
ss << "\tDescription:" << error.desc << "\n";

This comment has been minimized.

Copy link
@mliszcz

mliszcz May 20, 2019

Collaborator

Do you intend to use this operator in future as a generic utility for printing DevError or is it just for exception handler in dserver's init_hook? In former case, would concurrent access be a problem? In this example we will get mixed lines:

thread 1: std::cout << DevFailed{ ... } << std::cout;
thread 2: std::cout << DevFailed{ ... } << std::cout;

Buffering parts of the output in ostringstream before writing may help (probably depends on whether underlying write call is synchronized at OS level).

This comment has been minimized.

Copy link
@Ingvord

Ingvord May 22, 2019

Author Member

It is intended to use this operator on a stack trace local stringstream, as it is just a convenience function. I will mark it as inline.

Ingvord added some commits May 22, 2019

@Ingvord Ingvord added the help wanted label May 23, 2019

@Ingvord

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

Looks like even without auto Windows fails to compile simple for-loop:

3>.\dserver.cpp(486) : error C2143: syntax error : missing ',' before ':'
15843>.\dserver.cpp(487) : error C2143: syntax error : missing ';' before '{'
15853>.\dserver.cpp(488) : error C2143: syntax error : missing ',' before ':'
15863>.\dserver.cpp(489) : error C2143: syntax error : missing ';' before '{'
1587

where dserver.cpp#486

for (DeviceClass *dclass : this->get_class_list())

Any help from Windows expert is very well appreciated! Thanks.

@mliszcz

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@Ingvord, range-based for is c++11 feature (see: https://en.cppreference.com/w/cpp/language/range-for)

@bourtemb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@mliszcz is correct. You are using a C++ 11 feature. It's not Windows' fault, this time ;-) ...
I think after the release of cppTango 9.3.3, it is time to apply what was decided during the last Tango kernel meeting at Krakow.
If my notes are correct, it was decided to remove the support for MSVC9, MSVC10 and MSVC12.
So now that cppTango 9.3.3 is out, we could remove the appveyor builds for these compilers.
If some users still need support for these old compilers, they will need to maintain the compatibility in their own fork.

@Ingvord

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@mliszcz, @bourtemb thanks a lot for the clarification. I guess we need some macro here, i.e. if old windows then do it the old way with iterators, otherwise use range for. I will try to find existing pattern for this...

@bourtemb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

If you want to maintain the compatibility, you can indeed use something like:

#ifdef HAS_RANGE_BASE_FOR
// C++ 11 code
#else
// code for old compilers
#endif

You can search for #ifdef HAS_RANGE_BASE_FOR in the code to get some examples.
For instance in cppapi/client/devapi_base.cpp

@Ingvord

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@bourtemb , done in e52e2a8. A bit of a useless job as probably as this PR will be released for sure after 9.3.3 due to new virtual function. Let's see if AppVeyor is now happy...

@t-b t-b changed the title PR for #498 Add server_init_hook() method (#498) May 23, 2019

@bourtemb

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@reszelaz, could you please review/test this PR and tell us whether it fits your needs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.