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

convert make_pair to emplace #10646

Merged
merged 1 commit into from Oct 19, 2021
Merged

convert make_pair to emplace #10646

merged 1 commit into from Oct 19, 2021

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Aug 15, 2021

emplace* forwards the arguments directly. Avoids needing make_pair.

Signed-off-by: Rosen Penev rosenp@gmail.com

Short description

Checklist

I have:

  • [x ] read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • [x ] added or modified regression test(s)
  • [x ] added or modified unit test(s)
  • checked that this code was merged to master

@neheb neheb force-pushed the empla branch 4 times, most recently from 7646388 to d8649e6 Compare August 15, 2021 01:51
@neheb
Copy link
Contributor Author

neheb commented Aug 15, 2021

cool. compiles now.

pdns/decafsigners.cc Outdated Show resolved Hide resolved
pdns/decafsigners.cc Outdated Show resolved Hide resolved
pdns/decafsigners.cc Outdated Show resolved Hide resolved
pdns/decafsigners.cc Outdated Show resolved Hide resolved
@neheb neheb force-pushed the empla branch 3 times, most recently from 72acc54 to 786a839 Compare August 18, 2021 22:09
@neheb
Copy link
Contributor Author

neheb commented Aug 19, 2021

Fixed.

@omoerbeek
Copy link
Member

Sorry for forgetting about this PR. Can you rebase to fix the conflicts?

@neheb
Copy link
Contributor Author

neheb commented Oct 18, 2021

Rebased. This codebase is a mess. Lots of different styles in use. Maybe clang-format should be looked at.

@Habbie
Copy link
Member

Habbie commented Oct 18, 2021

We use clang-format except for files mentioned in .not-formatted. It's incremental work, eventually we'll get there - we don't want to break all PRs and backports in one go.

@neheb
Copy link
Contributor Author

neheb commented Oct 18, 2021

@Habbie just tried it. I assume I should be applying it to this PR?

@Habbie
Copy link
Member

Habbie commented Oct 18, 2021

'check-formatting - Your tests passed on CircleCI'. You did not break formatting in files we already have formatted, so there's no need for you to do anything here.

@neheb
Copy link
Contributor Author

neheb commented Oct 18, 2021

'check-formatting - Your tests passed on CircleCI'. You did not break formatting in files we already have formatted, so there's no need for you to do anything here.

hmm? git clang-format HEAD~1 says otherwise.

@Habbie
Copy link
Member

Habbie commented Oct 18, 2021

hmm? git clang-format HEAD~1 says otherwise.

I doubt that reads .not-formatted.

@neheb neheb force-pushed the empla branch 5 times, most recently from afe664d to 4ae8418 Compare October 18, 2021 21:55
emplace* forwards the arguments directly. Avoids needing make_pair.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads good. You converted some loops to range loops, but not all. The others can be done some later time,

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a tiny indentation issue that we could fix later. I'm also wondering why the change to alter the content in two loops?

pdns/dnsscope.cc Show resolved Hide resolved
pdns/rec_channel_rec.cc Show resolved Hide resolved
@rgacogne rgacogne merged commit d49753f into PowerDNS:master Oct 19, 2021
@neheb neheb deleted the empla branch October 19, 2021 15:09
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 this pull request may close these issues.

None yet

5 participants