This is an archive of the discontinued LLVM Phabricator instance.

For loop style fix
ClosedPublic

Authored by Prazek on Jul 22 2015, 7:09 PM.

Details

Reviewers
majnemer
rsmith

Diff Detail

Event Timeline

Prazek updated this revision to Diff 30442.Jul 22 2015, 7:09 PM
Prazek retitled this revision from to For loop style fix.
Prazek updated this object.
Prazek added a reviewer: rsmith.
Prazek added a subscriber: cfe-commits.
majnemer added inline comments.
lib/CodeGen/CGVTables.cpp
190

I'd name the variable BB instead because BasicBlock is the name of a class in LLVM.

Prazek added inline comments.Jul 22 2015, 11:42 PM
lib/CodeGen/CGVTables.cpp
190

that's why I don't like this convention :P

Prazek updated this revision to Diff 30497.Jul 23 2015, 9:47 AM
majnemer added inline comments.Jul 23 2015, 11:18 AM
lib/CodeGen/CGVTables.cpp
190–192

We all take issue with some part of the coding style but it is important that we all stick to it.

Prazek added inline comments.Jul 23 2015, 11:37 AM
lib/CodeGen/CGVTables.cpp
190–192

Yep, I hope my middle aproach with "BBblock" is similiar enough. Seriously name like BB for newbie like me doesn't give me any information more than 2 random letters

BB very frequently is BasicBlock and I is Instruction, typically in loops

for (... &BB : F)
  for (... &I : BB)

grep "&BB :" to see more examples. The variable name should really be BB to follow existing code conventions.

To clarify the type maybe it's better not using auto:

for (const BasicBlock &BB : F)

It there is a coding guide on using auto? I didn't find in the LLVM Programmer’s Manual.

Prazek updated this revision to Diff 30519.Jul 23 2015, 1:20 PM
Prazek marked 2 inline comments as done.Jul 23 2015, 9:11 PM
majnemer added inline comments.Jul 24 2015, 1:21 PM
lib/CodeGen/CGVTables.cpp
835

It would be nicer to use const CXXRecordDecl *RD : DeferredVTables because the type in the collection isn't trivially obvious. Also, the braces for the for-loop are now superfluous.

lib/CodeGen/ItaniumCXXABI.cpp
1026 ↗(On Diff #30519)

I'd change the auto to CXXBasePath because the type isn't clear.

1032 ↗(On Diff #30519)

Likewise, I'd change this auto to CXXBasePathElement.

Prazek marked 3 inline comments as done.Jul 24 2015, 3:01 PM
Prazek added inline comments.
lib/CodeGen/CGVTables.cpp
835

const CXXRecordDecl *const RD.

Prazek marked 2 inline comments as done.Jul 24 2015, 3:02 PM
Prazek updated this revision to Diff 30614.Jul 24 2015, 3:13 PM
majnemer added inline comments.Jul 24 2015, 3:31 PM
lib/CodeGen/CGVTables.cpp
835

I don't think it is typical LLVM style to stick const in that position, looks pretty verbose.

Prazek marked an inline comment as done.Jul 24 2015, 4:11 PM
Prazek updated this revision to Diff 30621.Jul 24 2015, 4:12 PM
majnemer accepted this revision.Jul 27 2015, 2:57 PM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Jul 27 2015, 2:57 PM
Prazek closed this revision.Jul 28 2015, 9:12 AM