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 ? 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 >>>>> >>>>> >>>>> >>>> >>> >> >
From 760c4ce0b47100c83d65320bdb081fbef6ae470a Mon Sep 17 00:00:00 2001 From: "Arnaud A. de Grandmaison" <[email protected]> Date: Thu, 1 May 2014 16:16:43 +0200 Subject: [PATCH] PR19601: testcase improvement The test can now catch all cases: - no removal of the 'no-integrated-as' flag - bogus removal of the flag, like when the remove_if was not followed by an erase --- test/Tooling/multi-jobs.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/Tooling/multi-jobs.cpp b/test/Tooling/multi-jobs.cpp index 36e3b34..980ade4 100644 --- a/test/Tooling/multi-jobs.cpp +++ b/test/Tooling/multi-jobs.cpp @@ -1,5 +1,6 @@ // RUN: not clang-check "%s" -- -no-integrated-as -c 2>&1 | FileCheck %s -// RUN: not clang-check "%s" -- -target x86_64-win32 -no-integrated-as -c 2>&1 | FileCheck %s +// The following test uses multiple time the same '-no-integrated-as' flag in order to make sure those flags are really skipped, and not just overwritten by luck : +// RUN: not clang-check "%s" -- -target x86_64-win32 -c -no-integrated-as -no-integrated-as -no-integrated-as 2>&1 | FileCheck %s // CHECK: C++ requires invalid; -- 1.8.3.2
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
