[PATCH] D12797: Refactor LoopConvertCheck.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:419
@@ +418,3 @@
+  // assumption the user is trying to modernize their codebase.
+  if (getLangOpts().CPlusPlus) {
+    Finder->addMatcher(makeArrayLoopMatcher(), this);
----------------
Now you can make that early exit!

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:494-495
@@ -493,4 +502,2 @@
     } else {
-      if (Descriptor.DerefByConstRef)
-        AutoRefType = Context->getConstType(AutoRefType);
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
----------------
I assume the change in the tests is due to the bug here? :)

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:497
@@ +496,3 @@
+    if (Descriptor.DerefByConstRef) {
+      AutoRefType = Context->getConstType(AutoRefType);
+      AutoRefType = Context->getLValueReferenceType(AutoRefType);
----------------
I'd give the intermediate result a different name.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:540
@@ -566,5 +539,3 @@
 
-  Confidence ConfidenceLevel(Finder.getConfidenceLevel());
-  if (FixerKind == LFK_Array) {
-    // The array being indexed by IndexVar was discovered during traversal.
-    ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
+  if (FixerKind == LFK_Iterator) {
+    // The iterator loops provides bound nodes to obtain this information.
----------------
I think we still want to split up this function. The if() {} else {} blocks are too long ...

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:541
@@ +540,3 @@
+  if (FixerKind == LFK_Iterator) {
+    // The iterator loops provides bound nodes to obtain this information.
+    const auto *InitVar = Nodes.getDeclAs<VarDecl>(InitVarName);
----------------
This sentence doesn't parse.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:549-551
@@ +548,5 @@
+    if (Descriptor.DerefByValue) {
+      // If the dereference operator returns by value then test for the
+      // canonical const qualification of the init variable type.
+      // TODO: Think about this.
+      Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
----------------
I think an example might help this comment. Here and elsewhere ;)

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:551
@@ +550,3 @@
+      // canonical const qualification of the init variable type.
+      // TODO: Think about this.
+      Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
----------------
LLVM uses FIXME; also, "Think about this" is not a good todo ;) They need to be more specific (or delete it).

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:580
@@ -581,1 +579,3 @@
+    if (usagesAreConst(Usages)) {
+      // If we can add a const, just do it.
       Descriptor.DerefByConstRef = true;
----------------
That comment doesn't give information, I think. I'd delete it.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:584-585
@@ -593,1 +583,4 @@
+      if (usagesReturnRValues(Usages)) {
+        // If the index usages (dereference, subscript, at, ...) return rvalues,
+        // then we should not use a reference.
         Descriptor.DerefByValue = true;
----------------
Perhaps add: , because we need to keep the code correct if it mutates the returned objects

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:590
@@ -596,8 +589,3 @@
         for (const Usage &U : Usages) {
-          if (!U.Expression || U.Expression->getType().isNull())
-            continue;
-          QualType Type = U.Expression->getType().getCanonicalType();
-          if (U.Kind == Usage::UK_MemberThroughArrow) {
-            if (!Type->isPointerType())
-              continue;
-            Type = Type->getPointeeType();
+          if (U.Expression && !U.Expression->getType().isNull()) {
+            QualType Type = U.Expression->getType().getCanonicalType();
----------------
I liked the early continue better...

================
Comment at: clang-tidy/modernize/LoopConvertCheck.h:34
@@ -32,2 +33,3 @@
     bool IsTriviallyCopyable;
+    StringRef ContainerString;
   };
----------------
I'd probably make that a std::string - having references in members is a frequent cause for bugs.


http://reviews.llvm.org/D12797