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.

Sign up for GitHub

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

Jump to bottom

Fixed some warnings #2306

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Fixed some warnings #2306

merged 2 commits into from
Dec 13, 2024

Conversation

mitza-oci
Copy link
Member

These warnings impact downstream projects since they're in headers and generated code.

@mitza-oci
Fixed warnings: reusing a name in nested scopes
aee8b43 
Updated style per guidelines
@mitza-oci
Fixed warnings in tao_idl generated code
70c1a78 
Classes that have a pure virtual member cannot be constructed as the
most-derived type of an object.  Their constructors are always called
indirectly from derived constructors.  Initializing virtual bases in
these constructors is effectively dead code since virtual bases can only
be initialized by the most-derived type.  Some compilers, with some
warning settings, generated warnings for this.
@jwillemsen
Copy link
Member

Hard to review, do you have some examples of the warnings?

@mitza-oci
Copy link
Member Author

Hard to review, do you have some examples of the warnings?

See commit comments 70c1a78 and log from GitHub Actions https://github.com/OpenDDS/OpenDDS/actions/runs/12284870208/job/34281667576?pr=4867#step:14:1283

@jwillemsen
Copy link
Member

Currently pretty busy, when this doesn't break a thing ok for me.

@mitza-oci
Copy link
Member Author

We can eventually add some CI builds in ACE_TAO that use higher warning levels.

@mitza-oci mitza-oci merged commit 5997007 into DOCGroup:master Dec 13, 2024
45 checks passed
@mitza-oci mitza-oci deleted the warnings branch December 13, 2024 15:45
mitza-oci added a commit to mitza-oci/ACE_TAO that referenced this pull request Dec 13, 2024
@mitza-oci
Merge pull request DOCGroup#2306 from mitza-oci/warnings
ca8a80c 
Fixed some warnings

(cherry picked from commit 5997007)

# Conflicts:
#	TAO/TAO_IDL/be/be_visitor_interface/interface_ss.cpp
@mitza-oci mitza-oci mentioned this pull request Dec 13, 2024
Draft
@jwillemsen
Copy link
Member

Our builds show a lot of new warnings, see for example https://download.remedy.nl/buildlogs/rhel80_acetao_debug

g++ -Wnon-virtual-dtor -fvisibility=hidden -fvisibility-inlines-hidden -std=c++17 -O3 -ggdb -m64 -pthread -fno-strict-aliasing -Wall -Wextra -Wpointer-arith -pipe -D_GNU_SOURCE   -D__ACE_INLINE__ -I/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/ACE -I../..  -c -o .obj/OneLineCosNamingS.o /home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp

/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp: In copy constructor â€کPOA_CosNaming::NamingContext::NamingContext(const POA_CosNaming::NamingContext&)’:
/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp:154:1: warning: base class â€کclass TAO_Abstract_ServantBase’ should be explicitly initialized in the copy constructor [-Wextra]

 POA_CosNaming::NamingContext::NamingContext (const NamingContext &)
 ^~~~~~~~~~~~~

@mitza-oci
Copy link
Member Author

Our builds show a lot of new warnings, see for example https://download.remedy.nl/buildlogs/rhel80_acetao_debug

g++ -Wnon-virtual-dtor -fvisibility=hidden -fvisibility-inlines-hidden -std=c++17 -O3 -ggdb -m64 -pthread -fno-strict-aliasing -Wall -Wextra -Wpointer-arith -pipe -D_GNU_SOURCE   -D__ACE_INLINE__ -I/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/ACE -I../..  -c -o .obj/OneLineCosNamingS.o /home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp

/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp: In copy constructor â€کPOA_CosNaming::NamingContext::NamingContext(const POA_CosNaming::NamingContext&)’:
/home/build/jenkins/workspace/rhel80_acetao_debug/BUILD/DOC_ROOT/TAO/tests/Bug_2424_Regression/OneLineCosNamingS.cpp:154:1: warning: base class â€کclass TAO_Abstract_ServantBase’ should be explicitly initialized in the copy constructor [-Wextra]

 POA_CosNaming::NamingContext::NamingContext (const NamingContext &)
 ^~~~~~~~~~~~~

So the different compilers are contradicting each-other? Maybe we need a preprocesor conditional.

@jwillemsen
Copy link
Member

Or maybe use C++11 default, but not sure about all implications, haven't looked into details of the warnings and all possible generations

@mitza-oci
Copy link
Member Author

How would default help?

@jwillemsen
Copy link
Member

I am on my phone without the generated code, but in taox11 we generate most copy/assingment as default so we let the compiler figure out the implementation, maybe that can be done in tao also?

@mitza-oci
Copy link
Member Author

For the default constructor, this->optable_ initialization would need to be moved to an in-class data member initializer.
I'm not sure why the copy constructor exists at all for skeletons, but it could probably be = default;

@jwillemsen
Copy link
Member

Did a quick a test on the Hello test, changing the copy constructor to = default resolved the warning in the Hello test on my local system (gcc 13)

@mitza-oci
Copy link
Member Author

Did a quick a test on the Hello test, changing the copy constructor to = default resolved the warning in the Hello test on my local system (gcc 13)

Was there a similar warning for the default constructor?

@jwillemsen
Copy link
Member

Only for the copy constructor at this moment

@mitza-oci mitza-oci mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrw972 jrw972 jrw972 approved these changes

@jwillemsen jwillemsen Awaiting requested review from jwillemsen

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mitza-oci @jwillemsen @jrw972