On Thu, May 1, 2014 at 7:47 AM, Arnaud Allard de Grandmaison < [email protected]> 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 <[email protected]>wrote: > >> On Wed, Apr 30, 2014 at 4:31 PM, Arnaud Allard de Grandmaison < >> [email protected]> 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 <[email protected]>wrote: >>> >>>> On Wed, Apr 30, 2014 at 3:40 PM, Arnaud Allard de Grandmaison < >>>> [email protected]> 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 >>>>> <[email protected]>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 <[email protected]> >>>>>> 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 < >>>>>> [email protected]> 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 >>>>>>> [email protected] >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
