On Thu, Aug 27, 2015 at 7:36 PM, Alexander Kornienko <ale...@google.com> wrote: > On Fri, Aug 28, 2015 at 1:23 AM, Aaron Ballman <aaron.ball...@gmail.com> > wrote: >> >> On Thu, Aug 27, 2015 at 7:20 PM, Alexander Kornienko <ale...@google.com> >> wrote: >> > alexfh added a comment. >> > >> > In http://reviews.llvm.org/D12081#234614, @aaron.ballman wrote: >> > >> >> While working on r246209, one of the build bots ran into an issue >> >> (commented below) that has me slightly perplexed. The build break can be >> >> found at: >> >> http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/30516 >> > >> > >> > Seems to be related to the -std=c++98 in the test? >> >> That's what I figured as well. >> >> > >> >> What's also strange is that I could not reproduce that failure locally >> >> (MSVC 2015 debug build, Windows 10)... >> > >> > >> > That's strange. Maybe there's some command-line argument parsing magic >> > when targeting windows? >> >> That's what I'm slightly more concerned by. I will investigate in the >> morning on my machine and see what I come up with. >> >> > >> > >> > ================ >> > Comment at: test/clang-tidy/modernize-use-nullptr-basic.cpp:2 >> > @@ +1,3 @@ >> > +// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-use-nullptr %t >> > -- \ >> > +// RUN: -std=c++98 -Wno-non-literal-null-conversion >> > +// REQUIRES: shell >> > ---------------- >> > aaron.ballman wrote: >> >> Sorry for bringing this up later, but how is this test supposed to >> >> work? nullptr is not a valid C++98 construct, and so I don't think we >> >> should >> >> be recommending fixes to use nullptr in this case. Is there a reason this >> >> test case is using -std=c++98 instead of -std=c++11? >> > Looks like a mistake. It should be -std=c++11. But while the check >> > wasn't looking at LangOpts, it didn't make any difference, because we don't >> > try to compile the fixed code. >> >> Okay, glad it's just a mistake. When I recommit my patch, I'll correct >> this RUN line. > > > Apparently, I was wrong. Some of the constructs in the test file fail to > compile in c++11. So maybe we should allow the check to run in c++98 mode: > to migrate the constructs that otherwise wouldn't compile. It still makes > sense to require C++, I guess.
I suppose that for things in the modernize category, it makes sense to allow it for any C++ language option as the user may be attempting to modernize the code base. I'll go that route, thank you! ~Aaron > >> >> >> Thanks! >> >> ~Aaron >> >> > >> > >> > http://reviews.llvm.org/D12081 >> > >> > >> > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
