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. 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.hl=85ct=xref_jump_to_defcl=GROKgsn=assert((data https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/ADT/StringRef.hl=76ct=xref_jump_to_defcl=GROKgsn=data | | length https://cs.corp.google.com/#piper///depot/google3/third_party/llvm/llvm/include/llvm/ADT/StringRef.hl=76ct=xref_jump_to_defcl=GROKgsn=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
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.
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.
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
