[PATCH] D12081: Add use-nullptr check to clang-tidy.

[Aaron Ballman via cfe-commits]

On Thu, Aug 27, 2015 at 7:36 PM, Alexander Kornienko <alexfh at google.com> wrote:
> On Fri, Aug 28, 2015 at 1:23 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Thu, Aug 27, 2015 at 7:20 PM, Alexander Kornienko <alexfh at 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
>> >
>> >
>> >
>
>