[PATCH] D13052: Add NamingStyle option to modernize-loop-convert.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

A few minor comments.


================
Comment at: clang-tidy/modernize/LoopConvertCheck.h:68
@@ -67,2 +67,3 @@
   Confidence::Level MinConfidence;
+  VariableNamer::NamingStyle NamingStyle;
 };
----------------
The variable can be const, the one above as well.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:820
@@ -822,1 +819,3 @@
+  size_t Len = ContainerName.size();
+  if (Len > 1 && ContainerName.endswith(Style == NS_UpperCase ? "S" : "s")) {
     IteratorName = ContainerName.substr(0, Len - 1);
----------------
No need to check the length. `endswith` handles this itself.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:841
@@ -828,4 +840,3 @@
 
-  IteratorName = ContainerName + "_" + OldIndex->getName().str();
-  if (!declarationExists(IteratorName))
-    return IteratorName;
+  auto AddSuffixToContainer = [&](std::string Suffix) -> std::string {
+    std::string Name = ContainerName;
----------------
Automatic captures always look suspicious and require more attention. I'd avoid them unless there's a significant improvement in readability. I'd also prefer a more generic function, say `AppendSuffixWithStyle`, which could then be placed next to the styles enumeration and other functions to work with banking styles.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.h:453
@@ -443,2 +452,3 @@
   bool declarationExists(llvm::StringRef Symbol);
+
 };
----------------
nit: please remove the empty line.


http://reviews.llvm.org/D13052