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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
