[PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
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.
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3903
if (Record.size() > 10 && Record[10] != 0)
-FunctionPrologues.push_back(std::make_pair(Func, Record[10]-1));
+FunctionPrologues.emplace_back(std::make_pair(Func, Record[10]-1));
No `make_pair`.
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3921
if (Record.size() > 13 && Record[13] != 0)
-FunctionPrefixes.push_back(std::make_pair(Func, Record[13]-1));
+FunctionPrefixes.emplace_back(std::make_pair(Func, Record[13]-1));
ditto
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3924
if (Record.size() > 14 && Record[14] != 0)
-FunctionPersonalityFns.push_back(std::make_pair(Func, Record[14] - 1));
+FunctionPersonalityFns.emplace_back(std::make_pair(Func, Record[14] -
1));
ditto
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3989
ValueList.push_back(NewGA);
- IndirectSymbolInits.push_back(std::make_pair(NewGA, Val));
+ IndirectSymbolInits.emplace_back(std::make_pair(NewGA, Val));
break;
ditto
Comment at: lib/Bitcode/Writer/ValueEnumerator.cpp:149
if (OM.lookup(U.getUser()).first)
- List.push_back(std::make_pair(, List.size()));
+ List.emplace_back(std::make_pair(, List.size()));
No `std::make_pair` needed.
Comment at: lib/Bitcode/Writer/ValueEnumerator.cpp:606
else
-Worklist.push_back(std::make_pair(Op, Op->op_begin()));
+Worklist.emplace_back(std::make_pair(Op, Op->op_begin()));
continue;
ditto
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:50
}
- Ranges.push_back(std::make_pair(, nullptr));
+ Ranges.emplace_back(std::make_pair(, nullptr));
}
No need for `std::make_pair`.
Comment at: lib/CodeGen/CGExprScalar.cpp:2755
BaseCheck->addIncoming(ValidBase, CheckShiftBase);
- Checks.push_back(std::make_pair(BaseCheck, SanitizerKind::ShiftBase));
+ Checks.emplace_back(std::make_pair(BaseCheck, SanitizerKind::ShiftBase));
}
`make_pair` can be removed, IIUC.
Comment at: lib/CodeGen/CGVTables.cpp:948
for (auto & : VTLayout.getAddressPoints())
-BitsetEntries.push_back(std::make_pair(AP.first.getBase(), AP.second));
+BitsetEntries.emplace_back(std::make_pair(AP.first.getBase(), AP.second));
Not sure if it's practical to teach the check this pattern, but
`std::make_pair` is not needed here.
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:1498
void visitCrossEdge(const SDep , const SUnit *Succ) {
-ConnectionPairs.push_back(std::make_pair(PredDep.getSUnit(), Succ));
+ConnectionPairs.emplace_back(std::make_pair(PredDep.getSUnit(), Succ));
}
No `make_pair` needed.
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:2079
}
- FuncInfo.PHINodesToUpdate.push_back(std::make_pair(MBBI++, Reg));
+ FuncInfo.PHINodesToUpdate.emplace_back(std::make_pair(MBBI++, Reg));
DbgLoc = DebugLoc();
No `make_pair` needed.
Repository:
rL LLVM
https://reviews.llvm.org/D21507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
Prazek added a comment. In http://reviews.llvm.org/D21507#465733, @sanjoy wrote: > 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. Repository: rL LLVM http://reviews.llvm.org/D21507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
sanjoy added a comment. 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)? Repository: rL LLVM http://reviews.llvm.org/D21507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
vsk added a comment.
In http://reviews.llvm.org/D21507#465444, @Prazek wrote:
> In http://reviews.llvm.org/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.
Repository:
rL LLVM
http://reviews.llvm.org/D21507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
Prazek added a comment.
In http://reviews.llvm.org/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
Repository:
rL LLVM
http://reviews.llvm.org/D21507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
vsk added a subscriber: vsk.
vsk added a comment.
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?
Repository:
rL LLVM
http://reviews.llvm.org/D21507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
Prazek added a comment. In http://reviews.llvm.org/D21507#462210, @jlebar wrote: > 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. Repository: rL LLVM http://reviews.llvm.org/D21507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21507: Changes after running check modernize-use-emplace (D20964)
jlebar added a subscriber: jlebar. jlebar added a comment. 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); Repository: rL LLVM http://reviews.llvm.org/D21507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
