Re: [clang] dd870f6 - Fix warning about unused std::unique result, erase shifted elements

2019-11-18 Thread David Blaikie via cfe-commits 
OK - thanks for the context!

On Mon, Nov 18, 2019 at 5:07 PM Ben Langmuir  wrote:

> Nice find.
>
> The change LGTM, and is clearly what I always intended it to do.  I was
> confused how this ever worked at all, but I see that (at least in libc++)
> we move the values into place rather than swap them, so the values at the
> end are still the largest values for types with a trivial move constructor.
>
> I am not working in this area these days and am not setup to provide a
> test case in any reasonable amount of time.  I think it would need a gtest,
> as I'm not sure this change is obvservable otherwise.
>
> Ben
>
> On Nov 18, 2019, at 4:44 PM, David Blaikie  wrote:
>
> Hey Ben - if you're still involved with this part of the project - could
> you check that this change (to code you committed back in 2014, r215810) is
> correct & add a test case if possible?
>
> On Thu, Nov 7, 2019 at 10:39 AM Reid Kleckner via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>>
>> Author: Reid Kleckner
>> Date: 2019-11-07T10:39:29-08:00
>> New Revision: dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571.diff
>>
>> LOG: Fix warning about unused std::unique result, erase shifted elements
>>
>> This is actually a functional change. I haven't added any new test
>> coverage. I'm just trying to fix the warning and hoping for the best.
>>
>> Added:
>>
>>
>> Modified:
>> clang/include/clang/Serialization/ContinuousRangeMap.h
>>
>> Removed:
>>
>>
>>
>>
>> 
>> diff  --git a/clang/include/clang/Serialization/ContinuousRangeMap.h
>> b/clang/include/clang/Serialization/ContinuousRangeMap.h
>> index 0c05537dd108..c2665c097416 100644
>> --- a/clang/include/clang/Serialization/ContinuousRangeMap.h
>> +++ b/clang/include/clang/Serialization/ContinuousRangeMap.h
>> @@ -118,14 +118,17 @@ class ContinuousRangeMap {
>>
>>  ~Builder() {
>>llvm::sort(Self.Rep, Compare());
>> -  std::unique(Self.Rep.begin(), Self.Rep.end(),
>> -  [](const_reference A, const_reference B) {
>> -// FIXME: we should not allow any duplicate keys, but there are
>> a lot of
>> -// duplicate 0 -> 0 mappings to remove first.
>> -assert((A == B || A.first != B.first) &&
>> -   "ContinuousRangeMap::Builder given non-unique keys");
>> -return A == B;
>> -  });
>> +  Self.Rep.erase(
>> +  std::unique(
>> +  Self.Rep.begin(), Self.Rep.end(),
>> +  [](const_reference A, const_reference B) {
>> +// FIXME: we should not allow any duplicate keys, but
>> there are
>> +// a lot of duplicate 0 -> 0 mappings to remove first.
>> +assert((A == B || A.first != B.first) &&
>> +   "ContinuousRangeMap::Builder given non-unique
>> keys");
>> +return A == B;
>> +  }),
>> +  Self.Rep.end());
>>  }
>>
>>  void insert(const value_type ) {
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] dd870f6 - Fix warning about unused std::unique result, erase shifted elements

2019-11-18 Thread Ben Langmuir via cfe-commits 
Nice find.

The change LGTM, and is clearly what I always intended it to do.  I was 
confused how this ever worked at all, but I see that (at least in libc++) we 
move the values into place rather than swap them, so the values at the end are 
still the largest values for types with a trivial move constructor.

I am not working in this area these days and am not setup to provide a test 
case in any reasonable amount of time.  I think it would need a gtest, as I'm 
not sure this change is obvservable otherwise.

Ben

> On Nov 18, 2019, at 4:44 PM, David Blaikie  wrote:
> 
> Hey Ben - if you're still involved with this part of the project - could you 
> check that this change (to code you committed back in 2014, r215810) is 
> correct & add a test case if possible?
> 
> On Thu, Nov 7, 2019 at 10:39 AM Reid Kleckner via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> 
> Author: Reid Kleckner
> Date: 2019-11-07T10:39:29-08:00
> New Revision: dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
>  
> 
> DIFF: 
> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571.diff
>  
> 
> 
> LOG: Fix warning about unused std::unique result, erase shifted elements
> 
> This is actually a functional change. I haven't added any new test
> coverage. I'm just trying to fix the warning and hoping for the best.
> 
> Added: 
> 
> 
> Modified: 
> clang/include/clang/Serialization/ContinuousRangeMap.h
> 
> Removed: 
> 
> 
> 
> 
> diff  --git a/clang/include/clang/Serialization/ContinuousRangeMap.h 
> b/clang/include/clang/Serialization/ContinuousRangeMap.h
> index 0c05537dd108..c2665c097416 100644
> --- a/clang/include/clang/Serialization/ContinuousRangeMap.h
> +++ b/clang/include/clang/Serialization/ContinuousRangeMap.h
> @@ -118,14 +118,17 @@ class ContinuousRangeMap {
> 
>  ~Builder() {
>llvm::sort(Self.Rep, Compare());
> -  std::unique(Self.Rep.begin(), Self.Rep.end(),
> -  [](const_reference A, const_reference B) {
> -// FIXME: we should not allow any duplicate keys, but there are a 
> lot of
> -// duplicate 0 -> 0 mappings to remove first.
> -assert((A == B || A.first != B.first) &&
> -   "ContinuousRangeMap::Builder given non-unique keys");
> -return A == B;
> -  });
> +  Self.Rep.erase(
> +  std::unique(
> +  Self.Rep.begin(), Self.Rep.end(),
> +  [](const_reference A, const_reference B) {
> +// FIXME: we should not allow any duplicate keys, but there 
> are
> +// a lot of duplicate 0 -> 0 mappings to remove first.
> +assert((A == B || A.first != B.first) &&
> +   "ContinuousRangeMap::Builder given non-unique keys");
> +return A == B;
> +  }),
> +  Self.Rep.end());
>  }
> 
>  void insert(const value_type ) {
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] dd870f6 - Fix warning about unused std::unique result, erase shifted elements

2019-11-18 Thread David Blaikie via cfe-commits 
Hey Ben - if you're still involved with this part of the project - could
you check that this change (to code you committed back in 2014, r215810) is
correct & add a test case if possible?

On Thu, Nov 7, 2019 at 10:39 AM Reid Kleckner via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

>
> Author: Reid Kleckner
> Date: 2019-11-07T10:39:29-08:00
> New Revision: dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
>
> URL:
> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
> DIFF:
> https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571.diff
>
> LOG: Fix warning about unused std::unique result, erase shifted elements
>
> This is actually a functional change. I haven't added any new test
> coverage. I'm just trying to fix the warning and hoping for the best.
>
> Added:
>
>
> Modified:
> clang/include/clang/Serialization/ContinuousRangeMap.h
>
> Removed:
>
>
>
>
> 
> diff  --git a/clang/include/clang/Serialization/ContinuousRangeMap.h
> b/clang/include/clang/Serialization/ContinuousRangeMap.h
> index 0c05537dd108..c2665c097416 100644
> --- a/clang/include/clang/Serialization/ContinuousRangeMap.h
> +++ b/clang/include/clang/Serialization/ContinuousRangeMap.h
> @@ -118,14 +118,17 @@ class ContinuousRangeMap {
>
>  ~Builder() {
>llvm::sort(Self.Rep, Compare());
> -  std::unique(Self.Rep.begin(), Self.Rep.end(),
> -  [](const_reference A, const_reference B) {
> -// FIXME: we should not allow any duplicate keys, but there are a
> lot of
> -// duplicate 0 -> 0 mappings to remove first.
> -assert((A == B || A.first != B.first) &&
> -   "ContinuousRangeMap::Builder given non-unique keys");
> -return A == B;
> -  });
> +  Self.Rep.erase(
> +  std::unique(
> +  Self.Rep.begin(), Self.Rep.end(),
> +  [](const_reference A, const_reference B) {
> +// FIXME: we should not allow any duplicate keys, but
> there are
> +// a lot of duplicate 0 -> 0 mappings to remove first.
> +assert((A == B || A.first != B.first) &&
> +   "ContinuousRangeMap::Builder given non-unique
> keys");
> +return A == B;
> +  }),
> +  Self.Rep.end());
>  }
>
>  void insert(const value_type ) {
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] dd870f6 - Fix warning about unused std::unique result, erase shifted elements

2019-11-07 Thread Reid Kleckner via cfe-commits 

Author: Reid Kleckner
Date: 2019-11-07T10:39:29-08:00
New Revision: dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571

URL: 
https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571
DIFF: 
https://github.com/llvm/llvm-project/commit/dd870f6929ee0b1dfb5fd000c7b826a1bd2d2571.diff

LOG: Fix warning about unused std::unique result, erase shifted elements

This is actually a functional change. I haven't added any new test
coverage. I'm just trying to fix the warning and hoping for the best.

Added: 


Modified: 
clang/include/clang/Serialization/ContinuousRangeMap.h

Removed: 




diff  --git a/clang/include/clang/Serialization/ContinuousRangeMap.h 
b/clang/include/clang/Serialization/ContinuousRangeMap.h
index 0c05537dd108..c2665c097416 100644
--- a/clang/include/clang/Serialization/ContinuousRangeMap.h
+++ b/clang/include/clang/Serialization/ContinuousRangeMap.h
@@ -118,14 +118,17 @@ class ContinuousRangeMap {
 
 ~Builder() {
   llvm::sort(Self.Rep, Compare());
-  std::unique(Self.Rep.begin(), Self.Rep.end(),
-  [](const_reference A, const_reference B) {
-// FIXME: we should not allow any duplicate keys, but there are a lot 
of
-// duplicate 0 -> 0 mappings to remove first.
-assert((A == B || A.first != B.first) &&
-   "ContinuousRangeMap::Builder given non-unique keys");
-return A == B;
-  });
+  Self.Rep.erase(
+  std::unique(
+  Self.Rep.begin(), Self.Rep.end(),
+  [](const_reference A, const_reference B) {
+// FIXME: we should not allow any duplicate keys, but there are
+// a lot of duplicate 0 -> 0 mappings to remove first.
+assert((A == B || A.first != B.first) &&
+   "ContinuousRangeMap::Builder given non-unique keys");
+return A == B;
+  }),
+  Self.Rep.end());
 }
 
 void insert(const value_type ) {



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits