-
Notifications
You must be signed in to change notification settings - Fork 380
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
Fix #2131 - Make -ORBListenerInterfaces work properly as documented and tested #2132
base: master
Are you sure you want to change the base?
Fix #2131 - Make -ORBListenerInterfaces work properly as documented and tested #2132
Conversation
I wouldn't put all gitignore files as part of this PR, when you want to add that, make that a separate PR. Also by using the C++ uniform initialization, std::unique_ptr, std instead of ACE_OS this could be more modern C++. I haven't looked at the issue in detail, this probably needs some detailed review, also the fact that you mention that your approach is maybe not the best will trigger review |
Done in #2133. The changes will disappear from here when that's merged and this is rebased.
Do you have specific callouts? I actually took pains to use the same techniques, tools, and styles used elsewhere in the code that I was changing, because it was not clear to me which things (like
I certainly welcome the review. I came up with the best solution that I knew how to, but I do not know this code nearly as well as do you, and it's possible that you just want to do it a different way, or maybe you'll even do a complete refactor of the affected areas to achieve other goals I'm not aware of. I resolved the Fuzz issues, and I worked through all the Codacy issues that I thought were valid. Two of them are clearly false positives caused by the use of the I'm waiting on platform-specific build results now. |
Should "ORBListener" by replaced by "ORBListen"? See |
Master requires c++14 or newer |
New test should be added to bin/lst file |
I just found |
a86a104
to
8cceee4
Compare
I've rebased to remove all the unrelated One thing I did notice through all of this work is that there are about half a dozen different places that use |
Please open a new issue for the getifaddrs/GetAdaptersAddresses proposal, that way it doesn't get lost. |
See the detailed explanation for this change in #2131.
A quick note about this change: it includes a new test,
$TAO_ROOT/orbsvcs/tests/Miop/McastListenerInterfaces
. In order for this test to compile and run,$TAO_ROOT/TAO_ACE.mwc
has to be edited to comment out or remove the lineorbsvcs/tests
in theexclude
block. It seems that allorbsvcs
tests are currently disabled inmaster
. I did not include this change in my pull request, because it seems those tests were disabled for a reason, which I won't second-guess. I know, for example, that most of the tests in$TAO_ROOT/orbsvcs/tests/Miop/
fail on a clean master.