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

[RFC] Test clang-tidy --checks modernize-use-emplace #29937

Closed
wants to merge 1 commit into from
Closed

[RFC] Test clang-tidy --checks modernize-use-emplace #29937

wants to merge 1 commit into from

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented May 21, 2020

@cmsbuild cmsbuild added this to the CMSSW_11_1_X milestone May 21, 2020
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@fwyzard fwyzard mentioned this pull request May 21, 2020
Closed
This was referenced May 21, 2020
Merged
Closed
Closed
Closed
Closed
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29937/15549

Code check has found code style and quality issues which could be resolved by applying following patch(s)

This was referenced Jun 26, 2020
Merged
Merged
Merged
Merged
Merged
Merged
@fwyzard
Copy link
Contributor Author

fwyzard commented Jun 30, 2020

IgnoreImplicitConstructors=1 may be better to reduce the number of somewhat unnecessary changes.
IMHO, it may be a bit burdensome to write code like in Alignment/MuonAlignmentAlgorithms/src/MuonResidualsAngleFitter.cc

  std::vector<std::string> parName;
  std::vector<double> step;
  parName.emplace_back("angle");
  step.push_back(0.1);

I guess the alternative is to make everything that compiles to use emplace_back, then the above will also need to have step.emplace_back(0.1);

Can you elaborate ?

The current code is something like

  std::vector<std::string> parName;
  std::vector<double> step;
  parName.push_back(std::string("angle"));
  step.push_back(0.1);

so setting IgnoreImplicitConstructors=1 shouldn't make any difference, since the constructor is explicit.

@slava77
Copy link
Contributor

slava77 commented Jun 30, 2020

My reading of IgnoreImplicitConstructors=1 in the docs was that the following will be kept unchanged

  std::vector<std::string> parName;
  std::vector<double> step;
  parName.push_back("angle");
  step.push_back(0.1);

instead of the current case where we get parName.emplace_back("angle");, while the next line stays with push_back

@fwyzard
Copy link
Contributor Author

fwyzard commented Jun 30, 2020 •
edited

I understood the same as you - but in the present code we have

  std::vector<std::string> parName;
  std::vector<double> step;
  parName.push_back(std::string("angle"));
  step.push_back(0.1);

which is not an implicit constructor - so I would expect that it will be changed to emplace_back() anyway ?

@fwyzard
Copy link
Contributor Author

fwyzard commented Jun 30, 2020

I did a simple test, starting with

#include <iostream>
#include <string>
#include <vector>

int main(void) {

  std::vector<int> ints;
  std::vector<std::string> strings;

  ints.push_back(42);
  ints.push_back(int{42});

  strings.push_back("Hello world");
  strings.push_back(std::string{"Hello world"});

  std::cout << ints.size() << " vs " << strings.size() << std::endl;

  return 0;
}

Running clang-tidy with modernize-use-emplace.IgnoreImplicitConstructors = 0 complains about

/home/fwyzard/test/emplace_back/test.cc:13:11: warning: use emplace_back instead of push_back [modernize-use-emplace]
  strings.push_back("Hello world");
          ^~~~~~~~~~
          emplace_back(
/home/fwyzard/test/emplace_back/test.cc:14:11: warning: use emplace_back instead of push_back [modernize-use-emplace]
  strings.push_back(std::string{"Hello world"});
          ^~~~~~~~~~~~~~~~~~~~~~             ~
          emplace_back(

Running clang-tidy with modernize-use-emplace.IgnoreImplicitConstructors = 1 complains only about

/home/fwyzard/test/emplace_back/test.cc:14:11: warning: use emplace_back instead of push_back [modernize-use-emplace]
  strings.push_back(std::string{"Hello world"});
          ^~~~~~~~~~~~~~~~~~~~~~             ~
          emplace_back(

So, using IgnoreImplicitConstructors = 1 does allow using strings.push_back("Hello world") .

@fwyzard
Copy link
Contributor Author

fwyzard commented Jun 30, 2020

As a separate test, it looks like emplace_back() is never turned back into puch_back().

@alja
Copy link
Contributor

alja commented Jun 30, 2020

+1

@mrodozov mrodozov mentioned this pull request Jul 1, 2020
Closed
This was referenced Jul 2, 2020
Merged
Closed
Merged
Merged
Merged
Merged
Merged
This was referenced Jul 9, 2020
Merged
Merged
Merged
Merged
Merged
Merged
@fwyzard fwyzard closed this Jul 14, 2020
@fwyzard fwyzard deleted the clang-tidy-modernize-use-emplace branch July 14, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
alca-pending analysis-approved code-checks-approved comparison-available core-pending daq-approved db-pending dqm-approved fastsim-approved generators-approved geometry-approved hlt-pending l1-pending orp-pending pending-signatures reconstruction-rejected simulation-approved tests-approved upgrade-approved visualization-approved xpog-pending
Projects
None yet
Milestone
CMSSW_11_2_X
Development

Successfully merging this pull request may close these issues.

None yet