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

[Richard Smith]

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

> Dumping Args before & after the remove_if shows that Jordan is right:
>  - before: Args: clang-tool -target x86_64 -no-integrated-as -c -v -c
> placeholder.cpp
>  - after: Args: clang-tool -target x86_64 -c -v -c placeholder.cpp
> placeholder.cpp
> and later on, _all_ input files are dropped. So we were just lucky.
>
> The code in stripPositionalArgs appends some arguments (-c
> placeholder.cpp), so I can not force -no-integrated-as to be last from the
> command line.
>
>  This would require to adapt stripPositionalArgs so that it prepends its
> arguments and we can test for the missing erase. Do we still want a
> testcase for that ? It seems a bit like an overshoot to me, but I aggree
> that there as been several incorrect commits in the same place.
>

Easier test: add -no-integrated-as *repeatedly*. If you add it enough
times, it'll be in the tail of unchanged elements left behind by remove_if.


> On Thu, May 1, 2014 at 12:46 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> 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/59de2fa2/attachment.html>