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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:815
@@ -815,3 +814,3 @@
   std::string IteratorName;
   std::string ContainerName;
   if (TheContainer)
----------------
This can be a StringRef to avoid some copies.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:820
@@ -820,3 +819,3 @@
   size_t Len = ContainerName.length();
   if (Len > 1 && ContainerName[Len - 1] == 's')
     IteratorName = ContainerName.substr(0, Len - 1);
----------------
Once `ContainerName` is a StringRef, this can be replaced with

  if (ContainerName.endswith("s"))

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:828
@@ -828,3 +827,3 @@
 
   IteratorName = ContainerName + "_" + OldIndex->getName().str();
   if (!declarationExists(IteratorName))
----------------
The underscore here is not needed in the LLVM style.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:832
@@ -832,3 +831,3 @@
 
   IteratorName = ContainerName + "_elem";
   if (!declarationExists(IteratorName))
----------------
This should also handle LLVM style.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:836
@@ -836,3 +835,3 @@
 
   IteratorName += "_elem";
   if (!declarationExists(IteratorName))
----------------
What will that be? `<container>_elem_elem`? Doesn't make sense to me.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:840
@@ -840,3 +839,3 @@
 
-  IteratorName = "_elem_";
+  IteratorName = (Style == NS_LLVM ? "Elem_" : "elem_");
 
----------------
The option with the trailing underscore doesn't make sense to me as well (unless we add a support for a style requiring trailing underscores for variable names).

================
Comment at: clang-tidy/modernize/LoopConvertUtils.cpp:844
@@ -844,3 +843,3 @@
   while (declarationExists(IteratorName))
     IteratorName += "i";
   return IteratorName;
----------------
I'd go with give_me_name{0,1,2,...} or something else that would make it more obvious that the automatic name selection failed and a user input is needed.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.h:418
@@ +417,3 @@
+  // Supported naming styles.
+  enum NamingStyle { NS_LLVM = 0, NS_Google = 1 };
+
----------------
The `IdentifierNamingCheck` defines styles in a more generic way (CamelCase, camelBack, lower_case, UPPER_CASE). It seems reasonable to use a similar set of options. Maybe even move the enum from that check to a common place together with the conversion routines.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.h:418
@@ +417,3 @@
+  // Supported naming styles.
+  enum NamingStyle { NS_LLVM = 0, NS_Google = 1 };
+
----------------
alexfh wrote:
> The `IdentifierNamingCheck` defines styles in a more generic way (CamelCase, camelBack, lower_case, UPPER_CASE). It seems reasonable to use a similar set of options. Maybe even move the enum from that check to a common place together with the conversion routines.
Please add tests for all naming conventions.

================
Comment at: clang-tidy/modernize/LoopConvertUtils.h:448
@@ -444,1 +447,3 @@
+
+  NamingStyle Style;
 };
----------------
Please make it const and put it right after `Context`.


http://reviews.llvm.org/D13052