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

[Arnaud Allard de Grandmaison]

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.


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/20140501/83f2195d/attachment.html>