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

Clang-tidy report Server::Instance invoke virtual function in constructor #8509

Closed
lambdai opened this issue Oct 6, 2019 · 1 comment · Fixed by #8510
Closed

Clang-tidy report Server::Instance invoke virtual function in constructor #8509

lambdai opened this issue Oct 6, 2019 · 1 comment · Fixed by #8510

Comments

@lambdai
Copy link
Contributor

lambdai commented Oct 6, 2019

Create this issue to avoid involving too many things in a single PR.

Not end of the world since the inheritance is only used by test but no production. And tests are passing.

Repro steps:
#8485

#4863

source/server/server.cc:406:3: note: Taking false branch
  if (bootstrap_.dynamic_resources().has_lds_config()) {
  ^
source/server/server.cc:418:46: note: Call to virtual function during construction
  Runtime::LoaderSingleton::get().initialize(clusterManager());
@lizan
Copy link
Member

lizan commented Oct 6, 2019

Yes this is legit clang-tidy error because virtual function invoked in constructor are bug-prone, (i.e. base class constructor cannot call virtual function overridden in derived class). Feel free to make clusterManager final in InstanceImpl.

lizan pushed a commit that referenced this issue Oct 7, 2019
Signed-off-by: Yuchen Dai [email protected]

Description:
Refactor InstanceImpl and remove the only derived test class of InstanceImpl
Mark entire Server::InstanceImpl as final to avoid accidentally invoke virtual function in InstanceImpl constructor.
Massage clang-tidy.

Fixes #8509

Signed-off-by: Yuchen Dai <[email protected]>
nandu-vinodan pushed a commit to nandu-vinodan/envoy that referenced this issue Oct 17, 2019
Signed-off-by: Yuchen Dai [email protected]

Description:
Refactor InstanceImpl and remove the only derived test class of InstanceImpl
Mark entire Server::InstanceImpl as final to avoid accidentally invoke virtual function in InstanceImpl constructor.
Massage clang-tidy.

Fixes envoyproxy#8509

Signed-off-by: Yuchen Dai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants