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

Thu May 1 12:44:53 PDT 2014

Thanks for the explanation.

Committed @ r207787

Cheers,
--
Arnaud

On Thu, May 1, 2014 at 9:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, May 1, 2014 at 7:47 AM, Arnaud Allard de Grandmaison <
> arnaud.adegm at gmail.com> wrote:
>
>> I modified the test so that it now catches all cases:
>>
>>    - no remove_if (and thus no erase) : fail
>>    - remove_if : fail
>>    - remove_if + erase : pass
>>
>> The test is however not robust in face of appending more options in the
>> stripPositionalArgs : for now it has the minimal number of
>> '-no-integrated-as' to catch the issues we have had, and we could add some
>> more to have a reasonnable margin. Thoughts ?
>>
>
> This is fine; there's not much chance of someone accidentally introducing
> the same bug for the same argument again. (We're really just guarding
> against your change being lost by a merge conflict or similar, and this
> test does that.)
>
>
>> Cheers,
>> --
>> Arnaud
>>
>>
>> On Thu, May 1, 2014 at 2:57 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> 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/20140501/b5e540b9/attachment.html>