[PATCH] D30226: [BranchFolding] Merge debug locations from common tail instead of removing

Taewook Oh via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 6 14:29:48 PST 2017


twoh added a comment.

@aprantl Thanks for the review! I added inline questions asking for your opinion about consistency vs guideline.



================
Comment at: lib/CodeGen/BranchFolding.cpp:758
+/// MergeCommonTailDebugLocs - Create merged DebugLocs of identical instructions
+/// across SameTails and assign it to the instruction in common tail.
+void BranchFolder::MergeCommonTailDebugLocs(unsigned commonTailIndex) {
----------------
aprantl wrote:
> doxygen comments should be in the header file and not repeat the method name.
All other functions in BranchFolding.cpp file have doxygen comments inside .cpp file with a method name. I thought it is better to keep the consistency, and wonder what is your opinion on this. 


================
Comment at: lib/CodeGen/BranchFolding.cpp:763
+  std::vector<MachineBasicBlock::iterator> NextCommonInsts(SameTails.size());
+  for (unsigned int i = 0 ; i != SameTails.size() ; ++i) {
+    if (i != commonTailIndex)
----------------
aprantl wrote:
> I think local variables should be captialized.
I kept 'i' small here for the consistency as well.


https://reviews.llvm.org/D30226





More information about the llvm-commits mailing list