[llvm-commits] [llvm] r158638 - in /llvm/trunk: include/llvm/ADT/DenseMap.h unittests/ADT/DenseMapTest.cpp

David Blaikie dblaikie at gmail.com
Mon Jun 18 15:44:48 PDT 2012


On Mon, Jun 18, 2012 at 3:27 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Jun 18, 2012 at 3:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Jun 18, 2012 at 9:19 AM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>> > On Mon, Jun 18, 2012 at 8:37 AM, Duncan Sands <baldrick at free.fr> wrote:
>> >>
>> >> Hi Chandler,
>> >>
>> >> > The codebase already has piles of these incorrect warnings in it,
>> >>
>> >> maybe clang does but LLVM proper does not, so why not keep it that way?
>> >
>> >
>> > Because the warning is broken. Fundamentally.
>> >
>> > There is no way to work around the warning that does not make the code
>> > worse. If the GCC optimizer cannot *prove* that the value is
>> > initialized, or
>> > does not hit one of its magical "don't warn on this" signals (usually
>> > escaped address to an external function), it calls it uninitialized.
>> >
>> > If you initialize it blindly, then any actual bug you might have in your
>> > code will fail to be reported by Valgrind and other tools that really
>> > can
>> > detect uninitialized uses.
>> >
>> > Now, maybe I've introduced a bug into this code and the warning is
>> > legitimate. I'll look into that.
>>
>> I believe it's this:
>>
>> diff --git include/llvm/ADT/DenseMap.h include/llvm/ADT/DenseMap.h
>> index 045b5c6..0166228 100644
>> --- include/llvm/ADT/DenseMap.h
>> +++ include/llvm/ADT/DenseMap.h
>> @@ -490,7 +490,7 @@ private:
>>
>>   template <typename LookupKeyT>
>>   bool LookupBucketFor(const LookupKeyT &Val, BucketT *&FoundBucket) {
>> -    const BucketT *ConstFoundBucket = FoundBucket;
>> +    const BucketT *ConstFoundBucket;
>
>
> I misread this about 10 times thinking this was part of the diff of this
> patch...

Ah, right, sorry about that.

> I see now that this your proposed *fix* for the warning. Sure, LGTM.

r158685.

> This at least doesn't *hide* any bad behavior unlike other cases of this warning....

The switch ones are the only ones I can't think of a good fix for -
adding a default case in the switch suppresses -Wswitch, adding a
default suppresses other warnings/Valgrind... I'll wait until this
comes back through the build I'll see what's still hanging about with
GCC's warnings.

- David




More information about the llvm-commits mailing list