This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix modernize-use-emplace to support alias cases
ClosedPublic

Authored by corona10 on Aug 25 2022, 2:37 AM.

Diff Detail

Event Timeline

corona10 created this revision.Aug 25 2022, 2:37 AM
corona10 requested review of this revision.Aug 25 2022, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 2:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I am new to this project. I know that I need to write a test also likewise: https://github.com/llvm/llvm-project/blob/b8655f7ff286b9ebcd97cdd24b9c8eb5b89b9651/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp#L872-L884
Please let me know how to generate CHECK-MESSAGES and CHECK-FIXES comments to this.

corona10 updated this revision to Diff 455536.Aug 25 2022, 4:30 AM
corona10 updated this revision to Diff 455544.Aug 25 2022, 5:09 AM

Add unittest

Can you mention this change in the Release Notes (clang-tools-extra/docs/ReleaseNotes.rst).

clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
1059

Can you please add a test like this, where emplace is called with a temporary object when the container type is an alias.

corona10 updated this revision to Diff 455807.Aug 25 2022, 11:57 PM
  • Update modernize-use-emplace to support emplacy cases for alias types.
  • Add new test
  • Update release note.
corona10 marked an inline comment as done.Aug 26 2022, 12:22 AM
corona10 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
1059

Done!

corona10 updated this revision to Diff 455819.Aug 26 2022, 12:59 AM
corona10 marked an inline comment as done.

Rebase

LGTM, just with a small nit.

LGTM, just with a small nit.

@njames93

Thank you but I can not find your comment about nit.
Would you like to check the comment one more time?

njames93 added inline comments.Aug 27 2022, 11:06 PM
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
138

Sorry about that, not sure what happened.
Anyway same goes for below, use a cxxRecordDecl matcher instead of namedDecl.

njames93 accepted this revision.Aug 27 2022, 11:06 PM
This revision is now accepted and ready to land.Aug 27 2022, 11:06 PM
corona10 updated this revision to Diff 456172.Aug 27 2022, 11:30 PM

Address code review.

corona10 marked an inline comment as done.Aug 27 2022, 11:54 PM

Thanks! I updated the change.

corona10 updated this revision to Diff 456173.Aug 27 2022, 11:56 PM
  • clang-format

@njames93 Can we land this patch before it occurs conflicting with other changes?

@njames93 Can we land this patch before it occurs conflicting with other changes?

Yes, sorry do you have commit access. If you don't what is your GitHub name and email that I should use when committing.

@njames93 Can we land this patch before it occurs conflicting with other changes?

Yes, sorry do you have commit access. If you don't what is your GitHub name and email that I should use when committing.

Thanks, I don't have a commit bit :)

Github name: corona10
email: donghee.na92@gmail.com

Thank you!

This revision was automatically updated to reflect the committed changes.