| Message ID | 20170218203048.22276-2-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 18/02/17 20:30, Luc Van Oostenryck wrote: > The existing test is an indirect test, using a warning > about context imbalance to show that some part of code > was discarded. > > Now that we have the minimal tools to test the output of > test-linearize, use them to replace the test by a direct one. Hmm, it may be a more direct test, but it is not clear just what is being tested (or indeed how it is being tested). > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > validation/c99-for-loop.c | 36 ++++++++++++------------------------ > 1 file changed, 12 insertions(+), 24 deletions(-) > > diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c > index 42246c513..427fde268 100644 > --- a/validation/c99-for-loop.c > +++ b/validation/c99-for-loop.c > @@ -1,33 +1,21 @@ > -int op(int); > - > -static int good(void) > +int c99(void); > +int c99(void) > { > - __context__(1); > - for (int i = 0; i < 10; i++) { > - if (!op(i)) { > - __context__(-1); > - return 0; > - } > - } > - __context__(-1); > - return 1; > -} > + int r = -1; > > -static int bad(void) > -{ > - __context__(1); > for (int i = 0; i < 10; i++) { > - if (!op(i)) { > - __context__(-1); > - return 0; > - } > + r = i; > } > - return 1; > + > + return r; > } > + > /* > * check-name: C99 for loop variable declaration > + * check-command: test-linearize $file > * > - * check-error-start > -c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block > - * check-error-end > + * check-output-ignore > + * check-output-contains: phisrc\\. > + * check-output-contains: phi\\. > + * check-output-contains: add\\. > */ > After applying this patch, I edited validation/c99-for-loop.c like so: $ git diff diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c index 427fde2..6b24fa8 100644 --- a/validation/c99-for-loop.c +++ b/validation/c99-for-loop.c @@ -2,8 +2,9 @@ int c99(void); int c99(void) { int r = -1; + int i; - for (int i = 0; i < 10; i++) { + for (i = 0; i < 10; i++) { r = i; } $ This modified test still passes (indeed the output is identical). :-P ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 18, 2017 at 10:37:36PM +0000, Ramsay Jones wrote: > > > On 18/02/17 20:30, Luc Van Oostenryck wrote: > > The existing test is an indirect test, using a warning > > about context imbalance to show that some part of code > > was discarded. > > > > Now that we have the minimal tools to test the output of > > test-linearize, use them to replace the test by a direct one. > > Hmm, it may be a more direct test, but it is not clear > just what is being tested (or indeed how it is being tested). > > > After applying this patch, I edited validation/c99-for-loop.c > like so: > > $ git diff > diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c > index 427fde2..6b24fa8 100644 > --- a/validation/c99-for-loop.c > +++ b/validation/c99-for-loop.c > @@ -2,8 +2,9 @@ int c99(void); > int c99(void) > { > int r = -1; > + int i; > > - for (int i = 0; i < 10; i++) { > + for (i = 0; i < 10; i++) { > r = i; > } > > $ > > This modified test still passes (indeed the output is identical). > :-P > > ATB, > Ramsay Jones Which is wonderful because it's exactly what it should be. In fact I would love to be able to do this: show that two versions of a program/function give exactly the same output, but the testsuite can't do that (yet). Now, you're right that It's may be not very clear what is being tested. I may make more sense once you replace it in the context of the patch it replace and the associated fix: - 0e91f878 ("validation: Check C99 for loop variables") - ed73fd32 ("linearize: Emit C99 declarations correctly") If you try the test case on the tree with the patch ed73fd32 reverted, you will see that the two versions doesn't give anymore the same result: The C89 version (your) version will still give the right output but the C99 version (the one of the test case) will essentialy gives an empty output (the code used to ignore the C99-style declaration and patch ed73fd32 fixed that). Be careful also to the fact that this test case depends on testsuite features only present on sparse-next, not on the master tree. Luc Van Oostenryck -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/02/17 01:10, Luc Van Oostenryck wrote: > On Sat, Feb 18, 2017 at 10:37:36PM +0000, Ramsay Jones wrote: >> >> >> On 18/02/17 20:30, Luc Van Oostenryck wrote: >>> The existing test is an indirect test, using a warning >>> about context imbalance to show that some part of code >>> was discarded. >>> >>> Now that we have the minimal tools to test the output of >>> test-linearize, use them to replace the test by a direct one. >> >> Hmm, it may be a more direct test, but it is not clear >> just what is being tested (or indeed how it is being tested). >> >> >> After applying this patch, I edited validation/c99-for-loop.c >> like so: >> >> $ git diff >> diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c >> index 427fde2..6b24fa8 100644 >> --- a/validation/c99-for-loop.c >> +++ b/validation/c99-for-loop.c >> @@ -2,8 +2,9 @@ int c99(void); >> int c99(void) >> { >> int r = -1; >> + int i; >> >> - for (int i = 0; i < 10; i++) { >> + for (i = 0; i < 10; i++) { >> r = i; >> } >> >> $ >> >> This modified test still passes (indeed the output is identical). >> :-P >> >> ATB, >> Ramsay Jones > > Which is wonderful because it's exactly what it should be. So why is it called c99-for-loop.c? ;-) [rhetorical question] > In fact I would love to be able to do this: show that two versions > of a program/function give exactly the same output, but the testsuite > can't do that (yet). OK > Now, you're right that It's may be not very clear what is being tested. Indeed! (So, the commit message could use some improvement, right?) > I may make more sense once you replace it in the context of the patch > it replace and the associated fix: > - 0e91f878 ("validation: Check C99 for loop variables") > - ed73fd32 ("linearize: Emit C99 declarations correctly") OK, so that sheds some light on the issue ... > If you try the test case on the tree with the patch ed73fd32 reverted, > you will see that the two versions doesn't give anymore the same result: > The C89 version (your) version will still give the right output > but the C99 version (the one of the test case) will essentialy gives > an empty output (the code used to ignore the C99-style declaration > and patch ed73fd32 fixed that). Yes, I can confirm this behaviour. If I 'git revert ed73fd32' then rebuild, sure enough the c99 version is 'essentially empty', but the c89 version is as before. So, how could I tell from the commit message alone, that this is what the test is doing? ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This serie adds some validations of C99-style for-loop initializers: scope and storage. Patch 1 replaces the current indirect test by a direct one Patch 2 adds a test case for the scope Patch 3 adds some test cases for storage. Patch 4 is a preparatory patch for 5). Patch 5 adds missing storage validation Changes since v1: - better log message for patch 1, thanks to Ramsay Jones Luc Van Oostenryck (5): replace test for c99 for-loop initializers add test case for scope of C99 for-loop declarations add test cases for storage of c99 for-loop declarations add a method to external_declaration() check the storage of C99 for-loop initializers lib.c | 2 +- parse.c | 37 ++++++++++++++++++++++++++++++------- parse.h | 3 ++- validation/c99-for-loop-decl.c | 40 ++++++++++++++++++++++++++++++++++++++++ validation/c99-for-loop.c | 36 ++++++++++++------------------------ 5 files changed, 85 insertions(+), 33 deletions(-) create mode 100644 validation/c99-for-loop-decl.c
diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c index 42246c513..427fde268 100644 --- a/validation/c99-for-loop.c +++ b/validation/c99-for-loop.c @@ -1,33 +1,21 @@ -int op(int); - -static int good(void) +int c99(void); +int c99(void) { - __context__(1); - for (int i = 0; i < 10; i++) { - if (!op(i)) { - __context__(-1); - return 0; - } - } - __context__(-1); - return 1; -} + int r = -1; -static int bad(void) -{ - __context__(1); for (int i = 0; i < 10; i++) { - if (!op(i)) { - __context__(-1); - return 0; - } + r = i; } - return 1; + + return r; } + /* * check-name: C99 for loop variable declaration + * check-command: test-linearize $file * - * check-error-start -c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block - * check-error-end + * check-output-ignore + * check-output-contains: phisrc\\. + * check-output-contains: phi\\. + * check-output-contains: add\\. */
The existing test is an indirect test, using a warning about context imbalance to show that some part of code was discarded. Now that we have the minimal tools to test the output of test-linearize, use them to replace the test by a direct one. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- validation/c99-for-loop.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-)