This is an archive of the discontinued LLVM Phabricator instance.

Changes after running check modernize-use-emplace (D20964)
AbandonedPublic

Authored by Prazek on Jun 19 2016, 12:49 PM.

Details

Summary

Many changes after running my new check. I coudn't generate diff with full files because it was too heavy.

NOTE before you comment:

  • there are some places where .push_back(std::make_pair(a, b)) is changed to .emplace_back(std::make_pair(a, b)). This is because

the type that make_pair returns is different than it is inside vector. I plan to write matcher that would match places when
.emplace_back(std::make_pair(a, b)) or .push_back(std::make_pair(a, b)) could be changed to .emplace_back(a, b). I don't want to change it by hand right now,
and I want to keep this diff like this, because I will have some code that I can try next check later.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 61218.Jun 19 2016, 12:49 PM
Prazek retitled this revision from to Changes after running check modernize-use-emplace (D20964).
Prazek updated this object.
Prazek added reviewers: alexfh, rsmith.
Prazek set the repository for this revision to rL LLVM.
Prazek added subscribers: cfe-commits, llvm-commits.
jlebar added a subscriber: jlebar.Jun 20 2016, 9:36 AM

There seem to be many nontrivial whitespace errors introduced by this patch. For example,

-        Attrs.push_back(HTMLStartTagComment::Attribute(Ident.getLocation(),
-                                                       Ident.getHTMLIdent()));
+        Attrs.emplace_back(Ident.getLocation(),
+                                                       Ident.getHTMLIdent());

and

-        BitGroups.push_back(BitGroup(LastValue, LastRLAmt, LastGroupStartIdx,
-                                     i-1));
+        BitGroups.emplace_back(LastValue, LastRLAmt, LastGroupStartIdx,
+                                     i-1);

There seem to be many nontrivial whitespace errors introduced by this patch. For example,

-        Attrs.push_back(HTMLStartTagComment::Attribute(Ident.getLocation(),
-                                                       Ident.getHTMLIdent()));
+        Attrs.emplace_back(Ident.getLocation(),
+                                                       Ident.getHTMLIdent());

and

-        BitGroups.push_back(BitGroup(LastValue, LastRLAmt, LastGroupStartIdx,
-                                     i-1));
+        BitGroups.emplace_back(LastValue, LastRLAmt, LastGroupStartIdx,
+                                     i-1);

That's right. I will git-clang-format it before pushing upstream. This is just raw diff of what check does.

vsk added a subscriber: vsk.Jun 22 2016, 11:03 AM

Neat! It would help to upload a git-clang-format'd. Fwiw I only managed to look over the changes in lib/{ARCMigrate,AST,Analysis}.

Have you run check-all and the full test-suite?

In D21507#464791, @vsk wrote:

Neat! It would help to upload a git-clang-format'd. Fwiw I only managed to look over the changes in lib/{ARCMigrate,AST,Analysis}.

Have you run check-all and the full test-suite?

Yep, didn't have any problems with dat

vsk added a comment.Jun 23 2016, 8:09 AM
In D21507#464791, @vsk wrote:

Neat! It would help to upload a git-clang-format'd. Fwiw I only managed to look over the changes in lib/{ARCMigrate,AST,Analysis}.

Have you run check-all and the full test-suite?

Yep, didn't have any problems with dat

Ok great! This looks good then. I'd wait for one of the clang-tidy devs to give an actual lgtm. It might also be worth sending a "heads-up" email to llvm-dev once it's approved.

One other thing to point out -- have you verified that the changes in unittests/ are benign (i.e. you're not semantically changing the tests)?

One other thing to point out -- have you verified that the changes in unittests/ are benign (i.e. you're not semantically changing the tests)?

Yes I do.

alexfh requested changes to this revision.Feb 2 2017, 8:44 AM

If you're still considering to submit this patch, could you rebase it (or maybe re-generate instead?) and split into easier to digest parts?

A couple of things I noticed:

  1. v.push_back(X); -> v.emplace_back(X); pattern, where X has a type of element of v. Not sure whether emplace_back provides any benefit in this case.
  2. a sub-case of 1 where X is std::make_pair(...), in this case emplace_back makes sense, if std::make_pair is removed as well. I don't know whether it's practical to teach the check this pattern. Given its frequency, might well be good.
lib/Bitcode/Reader/BitcodeReader.cpp
3903

No make_pair.

3921

ditto

3924

ditto

3989

ditto

lib/Bitcode/Writer/ValueEnumerator.cpp
149

No std::make_pair needed.

606

ditto

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp
50

No need for std::make_pair.

lib/CodeGen/CGExprScalar.cpp
2755

make_pair can be removed, IIUC.

lib/CodeGen/CGVTables.cpp
948

Not sure if it's practical to teach the check this pattern, but std::make_pair is not needed here.

lib/CodeGen/ScheduleDAGInstrs.cpp
1498

No make_pair needed.

lib/CodeGen/SelectionDAG/FastISel.cpp
2079

No make_pair needed.

This revision now requires changes to proceed.Feb 2 2017, 8:44 AM
Prazek abandoned this revision.May 6 2017, 2:40 AM