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