Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
alexfh closed this revision. alexfh added a comment. Thanks! Committed revision 245561. http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
bkramer accepted this revision. bkramer added a reviewer: bkramer. bkramer added a comment. This revision is now accepted and ready to land. LGTM, thanks! http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
angelgarcia updated this revision to Diff 32688.
angelgarcia added a comment.
Add a test.
http://reviews.llvm.org/D12186
Files:
clang-tidy/modernize/LoopConvertCheck.cpp
test/clang-tidy/Inputs/modernize-loop-convert/structures.h
test/clang-tidy/modernize-loop-convert-extra.cpp
Index: test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- test/clang-tidy/modernize-loop-convert-extra.cpp
+++ test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -605,4 +605,25 @@
}
}
+} // namespace SingleIterator
+
+
+namespace Macros {
+
+const int N = 10;
+int arr[N];
+
+void messing_with_macros() {
+ for (int i = 0; i < N; ++i) {
+printf("Value: %d\n", arr[i]);
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto & elem : arr) {
+ // CHECK-FIXES-NEXT: printf("Value: %d\n", elem);
+
+ for (int i = 0; i < N; ++i) {
+printf("Value: %d\n", CONT arr[i]);
+ }
}
+
+} // namespace Macros
Index: test/clang-tidy/Inputs/modernize-loop-convert/structures.h
===
--- test/clang-tidy/Inputs/modernize-loop-convert/structures.h
+++ test/clang-tidy/Inputs/modernize-loop-convert/structures.h
@@ -176,4 +176,15 @@
iterator begin() const;
iterator end() const;
};
+
+namespace Macros {
+
+struct MacroStruct {
+ int arr[10];
+};
+static MacroStruct *MacroSt;
+#define CONT MacroSt->
+
+} // namespace Macros
+
#endif // STRUCTURES_H
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -337,8 +337,9 @@
const LangOptions &LangOpts,
SourceRange Range) {
if (SourceMgr.getFileID(Range.getBegin()) !=
- SourceMgr.getFileID(Range.getEnd()))
-return nullptr;
+ SourceMgr.getFileID(Range.getEnd())) {
+return StringRef(); // Empty string.
+ }
return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr,
LangOpts);
Index: test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- test/clang-tidy/modernize-loop-convert-extra.cpp
+++ test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -605,4 +605,25 @@
}
}
+} // namespace SingleIterator
+
+
+namespace Macros {
+
+const int N = 10;
+int arr[N];
+
+void messing_with_macros() {
+ for (int i = 0; i < N; ++i) {
+printf("Value: %d\n", arr[i]);
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto & elem : arr) {
+ // CHECK-FIXES-NEXT: printf("Value: %d\n", elem);
+
+ for (int i = 0; i < N; ++i) {
+printf("Value: %d\n", CONT arr[i]);
+ }
}
+
+} // namespace Macros
Index: test/clang-tidy/Inputs/modernize-loop-convert/structures.h
===
--- test/clang-tidy/Inputs/modernize-loop-convert/structures.h
+++ test/clang-tidy/Inputs/modernize-loop-convert/structures.h
@@ -176,4 +176,15 @@
iterator begin() const;
iterator end() const;
};
+
+namespace Macros {
+
+struct MacroStruct {
+ int arr[10];
+};
+static MacroStruct *MacroSt;
+#define CONT MacroSt->
+
+} // namespace Macros
+
#endif // STRUCTURES_H
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -337,8 +337,9 @@
const LangOptions &LangOpts,
SourceRange Range) {
if (SourceMgr.getFileID(Range.getBegin()) !=
- SourceMgr.getFileID(Range.getEnd()))
-return nullptr;
+ SourceMgr.getFileID(Range.getEnd())) {
+return StringRef(); // Empty string.
+ }
return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr,
LangOpts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
angelgarcia added a comment. In http://reviews.llvm.org/D12186#228704, @bkramer wrote: > I meant the code before your change, which calls `StringRef(const char *Str)` > and completely disallows nullptr. In other words: this change is missing a > regression test. You are right, the current tests don't cover this line. > I prefer the default ctor. Using nullptr for the empty string is just an > implementation detail, exposing it doesn't add clarity in my opinion. OK. I'll put the default constructor and try to write a test that covers this case. Thank you for your comments! http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
bkramer added a comment. In http://reviews.llvm.org/D12186#228702, @angelgarcia wrote: > It is allowed as long as you specify that the length is 0. I meant the code before your change, which calls `StringRef(const char *Str)` and completely disallows nullptr. In other words: this change is missing a regression test. > I thought that this way it was clear that I was returning an empty > string, but they are both equivalent. I can change it if you think the > default constructor is better. I prefer the default ctor. Using nullptr for the empty string is just an implementation detail, exposing it doesn't add clarity in my opinion. http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
angelgarcia added a comment. Oooops, copy pasting there was not a good idea. Sorry :( http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
angelgarcia added a comment. It is allowed as long as you specify that the length is 0. assert https://cs.corp.google.com/#piper///depot/google3/third_party/grte/v4_x86/release/usr/grte/v4/include/assert.h&l=85&ct=xref_jump_to_def&cl=GROK&gsn=assert((data https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/ADT/StringRef.h&l=76&ct=xref_jump_to_def&cl=GROK&gsn=data | | length https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/ADT/StringRef.h&l=76&ct=xref_jump_to_def&cl=GROK&gsn=length | 0) &&"StringRef cannot be built from a NULL argument with - non-null length"); I thought that this way it was clear that I was returning an empty string, but they are both equivalent. I can change it if you think the default constructor is better. http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12186: Fix bug in modernize-loop-convert check.
bkramer added a subscriber: bkramer. bkramer added a comment. Is this tested? I'd expect a crash when constructing a StringRef implicitly from nullptr. Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:341 @@ -340,3 +340,3 @@ SourceMgr.getFileID(Range.getEnd())) -return nullptr; +return StringRef(nullptr, 0); You can just use the default ctor `return StringRef();` http://reviews.llvm.org/D12186 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
