r207696 - PR19601: std::remove_if does not really remove the elements.

[Richard Smith]

On Wed, Apr 30, 2014 at 3:40 PM, Arnaud Allard de Grandmaison <
arnaud.adegm at gmail.com> wrote:

> I have not been able to come up with a testcase which does better than
> what test/Tooling/multi-jobs.cpp already does.
>
> The current test status:
>  1. no remove_if (and thus no erase) : multi-jobs fails
>  2. remove_if : multi-jobs pass
>  3. remove_if + erase : multi-jobs pass
>
> Invoking clang-check with '-v' shows that in cases 2 & 3 the
> -no-integrated-as has been dropped. I feel a bit like hunting a red herring
> there : depending on the argument position, it gets dropped or not.
> Jordan's hypothesis is probably right.
>

Try putting it at the end of the command line.


> Cheers,
> --
> Arnaud
>
>
>
>
>
> On Wed, Apr 30, 2014 at 11:30 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>
>> My hypothesis for how this was working: std::remove_if reorders elements
>> using std::move, so we were getting lucky that -no-integrated-as wasn't the
>> last argument and something else got moved on top of it. We'd have
>> duplicates of the last few arguments (because they got moved earlier), but
>> that wouldn't show -no-integrated-as.
>>
>> Jordan
>>
>>
>> On Apr 30, 2014, at 14:09, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> This needs a test case. This code has already been committed,
>> incorrectly, twice before this revision; the existing test case clearly
>> doesn't actually cover it properly.
>>
>>
>> On Wed, Apr 30, 2014 at 12:59 PM, Arnaud A. de Grandmaison <
>> arnaud.adegm at gmail.com> wrote:
>>
>>> Author: aadg
>>> Date: Wed Apr 30 14:59:22 2014
>>> New Revision: 207696
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=207696&view=rev
>>> Log:
>>> PR19601: std::remove_if does not really remove the elements.
>>>
>>> It moves them at the end of the range instead, so an extra erase is
>>> needed.
>>>
>>> It is strange that this code works without the erase. On the other hand,
>>> removing the remove_if will make fail some tests.
>>>
>>> Modified:
>>>     cfe/trunk/lib/Tooling/CompilationDatabase.cpp
>>>
>>> Modified: cfe/trunk/lib/Tooling/CompilationDatabase.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/CompilationDatabase.cpp?rev=207696&r1=207695&r2=207696&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Tooling/CompilationDatabase.cpp (original)
>>> +++ cfe/trunk/lib/Tooling/CompilationDatabase.cpp Wed Apr 30 14:59:22
>>> 2014
>>> @@ -238,8 +238,9 @@ static bool stripPositionalArgs(std::vec
>>>
>>>    // Remove -no-integrated-as; it's not used for syntax checking,
>>>    // and it confuses targets which don't support this option.
>>> -  std::remove_if(Args.begin(), Args.end(),
>>> -                 MatchesAny(std::string("-no-integrated-as")));
>>> +  Args.erase(std::remove_if(Args.begin(), Args.end(),
>>> +
>>>  MatchesAny(std::string("-no-integrated-as"))),
>>> +             Args.end());
>>>
>>>    const std::unique_ptr<driver::Compilation> Compilation(
>>>        NewDriver->BuildCompilation(Args));
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140430/f6da24e8/attachment.html>