Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
felix.winkelm...@bevuta.com writes: >> '(list a123) -> `(list ,(gensym 'a123)), where the symbol a123 is the >> name of the TV. This symbol is used to store a value in the unification >> trail. > > Note that you could store the original name on the plist of the gensym, > then deref the chain of gensyms/orig names when printing. That's what Peter suggested too. I guess it isn't too bad performance-wise: (import (chicken base) (chicken time) (chicken plist)) (define (rename v) (let* ([orig (or (get v 'orig-name #f) (strip-syntax v))] [new (gensym orig)]) (put! new 'orig-name orig) new)) (let ([t (current-milliseconds)]) (let lp ([v 'a] [i 0]) (if (< i 100) (lp (rename v) (add1 i)) (print (- (current-milliseconds) t) " " (get v 'orig-name) ;; $ csc -O3 gensym.scm && ./gensym ;; 377 a > > > felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> '(list a123) -> `(list ,(gensym 'a123)), where the symbol a123 is the > name of the TV. This symbol is used to store a value in the unification > trail. Note that you could store the original name on the plist of the gensym, then deref the chain of gensyms/orig names when printing. felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Evan Hanson writes: > Hi folks, > Hi Evan! [snip] > * Patch 0012 - I don't think the name provides much benefit, and in > any case the way it was printed (with parens) was visually > confusing. Better to use "; " or the like, later. I didn't like that either. I just didn't have the guts to drop information present in the old messages. I think the name part could be dropped completely from the procedure types without losing any information. The relevant names can be dug up from the nodes at hand. > > * Patch 0017 - I think the effect of this patch is important, but I > still feel that mapping back to the user-specified names is better > than regex-based number stripping, to avoid munging tvs that really > do include a number. It feels there would have to be some serious identifier renaming black magic to get the original names back after being renamed by simplify-types a million times. I've tried to fix this but failed. This is an opportunity to think about alternative for the current type variable renaming scheme. I think the gensym version is not the right way. I'd like to replace the gensym with a simple (module internal) global fixnum counter. This would leave strip-syntax purely for printing messages to the user. Currently a TV is renamed effectively like this: '(list a123) -> `(list ,(gensym 'a123)), where the symbol a123 is the name of the TV. This symbol is used to store a value in the unification trail. One alternative representation for a TV would be a simple list: '(list (typevar 1 a123)) -> `(list (typevar ,(next-counter!) a123)) Here the current 'a123 is replaced with (typevar original-name). The is used for the unification trail lookups. The alternative I would use is the mutating record version I have described in some earlier post (in the discussions about the recent renaming fixes). This would help to simplify the scrutinizer in the future by removing the typeenv. I can make patch for this if this sounds OK. > > Anyway, the best way to understand the effect of this patch set is > probably not to go through each patch individually (I've already done > that, so you don't have to!) but rather to have a look at the "expected" > files for the scrutiny tests to see what the resulting output looks > like. Compare them to what we have currently and I think you'll see a > nice improvement. > > Cheers, > > Evan > > ___ > Chicken-hackers mailing list > Chicken-hackers@nongnu.org > https://lists.nongnu.org/mailman/listinfo/chicken-hackers ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> I've just pushed most of these patches, with signoffs, to a branch > called "scrutiny-message-formatting", and I think we should merge it. I have merged the branch into master. The messages are nice, but may be a bit over the top, regarding verbosity. Still, it is an improvement. felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
felix.winkelm...@bevuta.com writes: >> Hi folks, >> >> I've just pushed most of these patches, with signoffs, to a branch >> called "scrutiny-message-formatting", and I think we should merge it. > > Thanks for doing this, I've ran the tests and so far things look good. > > I'm a bit concerned about the verbosity of the warnings. For generated > code or for macro expansions, cases like > > (if #f ...) > > or > > (let ((a '(x . y))) > (if (pair? a) ...)) > > will generate lots of output that only applies to trivially optimizable > cases. I'm fine with merging the patches but perhaps we should > distinguish between true errors (that can't possibly work) and > those warnings that apply to valid code but indicate redundancies. > Hi Felix, I totally agree about these two cases. I'd vote for hiding these messages altogether. Even if you're compiling with -verbose. Maybe just show statistics for known predicate calls with statically known results. There's already a statistic for dropped branches. And I agree that the scrutinizer should only show warnings about expressions it knows for sure are wrong. ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> Hi folks, > > I've just pushed most of these patches, with signoffs, to a branch > called "scrutiny-message-formatting", and I think we should merge it. Thanks for doing this, I've ran the tests and so far things look good. I'm a bit concerned about the verbosity of the warnings. For generated code or for macro expansions, cases like (if #f ...) or (let ((a '(x . y))) (if (pair? a) ...)) will generate lots of output that only applies to trivially optimizable cases. I'm fine with merging the patches but perhaps we should distinguish between true errors (that can't possibly work) and those warnings that apply to valid code but indicate redundancies. felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Hi folks, I've just pushed most of these patches, with signoffs, to a branch called "scrutiny-message-formatting", and I think we should merge it. I actually reviewed all of these some time ago, and while there are still some changes and additions I'd like to include, I haven't had time to continue working on it for several months and don't want to block all this good work (and it is good work!) from going in. I have made some small changes to the patches, mostly cosmetic changes to message wording, indentation, parameter names and the like. I also added two small commits and simplified the commit messages to a degree that made sense to me. I've also omitted two of the patches where I think we should use a different approach: * Patch 0012 - I don't think the name provides much benefit, and in any case the way it was printed (with parens) was visually confusing. Better to use "; " or the like, later. * Patch 0017 - I think the effect of this patch is important, but I still feel that mapping back to the user-specified names is better than regex-based number stripping, to avoid munging tvs that really do include a number. Anyway, the best way to understand the effect of this patch set is probably not to go through each patch individually (I've already done that, so you don't have to!) but rather to have a look at the "expected" files for the scrutiny tests to see what the resulting output looks like. Compare them to what we have currently and I think you'll see a nice improvement. Cheers, Evan ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
On 2019-01-12 14:49, megane wrote: > Here's a modified patch that does these two things. Thanks, here's a sign-off (also updated to work on Windows) as well as the first patch again so they can be applied in sequence. > It occurred to me that perhaps redact-gensyms shouldn't use XXX as the > replacement as that's used in comments in the codebase. It could make > grepping for XXXs a bit painful. Perhaps, but let's not worry about that right now; `git grep XXX *.scm` is fine for the meantime. Cheers, Evan >From 18e468270b5a25f8bd79fd88b7046fb5b74ca6c6 Mon Sep 17 00:00:00 2001 From: megane Date: Mon, 19 Nov 2018 10:01:33 +0200 Subject: [PATCH 1/2] Add new test for scrutinizer message formatting This new test covers most, but not all, scrutinizer messages. In the test scripts, scrutiny-tests-2.scm has been moved up so that all output is generated before diffing anything. Signed-off-by: Evan Hanson --- distribution/manifest | 2 + tests/runtests.bat| 10 +- tests/runtests.sh | 6 +- tests/scrutinizer-message-format.expected | 238 ++ tests/test-scrutinizer-message-format.scm | 77 ++ 5 files changed, 329 insertions(+), 4 deletions(-) create mode 100644 tests/scrutinizer-message-format.expected create mode 100644 tests/test-scrutinizer-message-format.scm diff --git a/distribution/manifest b/distribution/manifest index 9c1dcfdb..c1a615ea 100644 --- a/distribution/manifest +++ b/distribution/manifest @@ -176,6 +176,8 @@ tests/scrutiny-tests-2.scm tests/scrutiny-tests-3.scm tests/scrutiny.expected tests/scrutiny-2.expected +tests/test-scrutinizer-message-format.scm +tests/scrutinizer-message-format.expected tests/syntax-rule-stress-test.scm tests/syntax-tests.scm tests/syntax-tests-2.scm diff --git a/tests/runtests.bat b/tests/runtests.bat index f739a297..47d80c0c 100644 --- a/tests/runtests.bat +++ b/tests/runtests.bat @@ -92,19 +92,25 @@ echo scrutiny tests ... if errorlevel 1 exit /b 1 a.out if errorlevel 1 exit /b 1 + %compile% scrutiny-tests.scm -A -verbose 2>scrutiny.out if errorlevel 1 exit /b 1 rem this is sensitive to gensym-names, so make it optional if not exist scrutiny.expected copy /Y scrutiny.out scrutiny.expected -fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out +%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out if errorlevel 1 exit /b 1 +%compile% test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out +rem this is expected to fail, so no errorlevel check -%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out +fc /lb%FCBUFSIZE% /w scrutinizer-message-format.expected scrutinizer-message-format.out +if errorlevel 1 exit /b 1 +fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out if errorlevel 1 exit /b 1 if not exist scrutiny-2.expected copy /Y scrutiny-2.out scrutiny-2.expected + fc /lb%FCBUFSIZE% /w scrutiny-2.expected scrutiny-2.out if errorlevel 1 exit /b 1 diff --git a/tests/runtests.sh b/tests/runtests.sh index 6da7630d..c6f9252e 100755 --- a/tests/runtests.sh +++ b/tests/runtests.sh @@ -123,11 +123,13 @@ if test \! -f specialization.expected; then cp specialization.expected specialization.out fi +$compile scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out +$compile test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out || true + +diff $DIFF_OPTS scrutinizer-message-format.expected scrutinizer-message-format.out diff $DIFF_OPTS scrutiny.expected scrutiny.out diff $DIFF_OPTS specialization.expected specialization.out -$compile scrutiny-tests-2.scm -A 2>scrutiny-2.out -verbose - # this is sensitive to gensym-names, so make it optional if test \! -f scrutiny-2.expected; then cp scrutiny-2.expected scrutiny-2.out diff --git a/tests/scrutinizer-message-format.expected b/tests/scrutinizer-message-format.expected new file mode 100644 index ..9c7299f7 --- /dev/null +++ b/tests/scrutinizer-message-format.expected @@ -0,0 +1,238 @@ + +Warning: literal in operator position: (1 2) + +Warning: literal in operator position: (1 2) + +Warning: in toplevel procedure `r-proc-call-argument-count-mismatch': + (test-scrutinizer-message-format.scm:9) in procedure call to `scheme#cons', expected 2 arguments but was given 1 argument + +Warning: in toplevel procedure `r-proc-call-argument-type-mismatch': + (test-scrutinizer-message-format.scm:10) in procedure call to `scheme#length', expected argument #1 of type `list' but was given an argument of type `symbol' + +Warning: in toplevel procedure `r-proc-call-argument-value-count': + (test-scrutinizer-message-format.scm:11) expected a single result in argument #1 of procedure call `(scheme#list (chicken.time#cpu-time))', but received 2 results + +Warning: in toplevel procedure `r-proc-call-argument-value-count': + (test-scrutinizer-message-format.scm:11) expected a single result in arg
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Evan Hanson writes:
> On 2019-01-10 18:12, megane wrote:
>> Evan Hanson writes:
[snip]
>> Do you agree with approach I took about gensym'd variables in the second
>> patch? If not, I think I'll have to come up with something else.
>
> That's a nice idea, I think it's probably fine. But if we're going to do
> that, why not wipe line numbers in all the expected files rather than
> just the new one?
>
Good idea.
>> diff --git a/tests/redact-gensyms.scm b/tests/redact-gensyms.scm
>> new file mode 100644
>> index 000..c6abb7b
>> --- /dev/null
>> +++ b/tests/redact-gensyms.scm
>> @@ -0,0 +1,25 @@
>> + (define prefixes (if (null? (command-line-arguments))
>> +'("tmp" "g")
>> +(string-split (car (command-line-arguments)) ",")))
>> +
>> + (let ((rege (irregex `(: ($ (or ,@prefixes)) (+ numeric)
>
> This regex could be `(: bow ($ (or ,@prefixes)) (+ numeric)) so you avoid
> accidentally replacing numbers in something like a variable called "dog1".
>
Didn't think of that, good catch.
Here's a modified patch that does these two things.
It occurred to me that perhaps redact-gensyms shouldn't use XXX as the
replacement as that's used in comments in the codebase. It could make
grepping for XXXs a bit painful.
>From 999b2534cff88a13f040a23b9e7bec1c1c8ca351 Mon Sep 17 00:00:00 2001
From: megane
Date: Wed, 28 Nov 2018 17:28:03 +0200
Subject: [PATCH] tests/runtests.sh: Sanitize gensyms from scrutinizer outputs
Instead of skipping tests that are sensitive to gensyms altogether try
to sanitize the output.
For scrutinizer-message-format.scm, sanitize a and b because they are
used in typevars. These are removed once the scrutinizer sanitizes
typevars in messages internally again (fix for #1563 broke this).
Also, sanitize "scm:" so line number mismatches do not cause million
diff conflicts when adding/removing stuff to/from tests.
* tests/redact-gensyms.scm: New small program to replace numbers from
common gensym prefixes
* [tests] runtests.sh: compile, use redact-gensyms
---
distribution/manifest | 1 +
tests/redact-gensyms.scm | 25 +++
tests/runtests.sh | 31 +++-
tests/scrutinizer-message-format.expected | 42 ++-
tests/scrutiny-2.expected | 46 ++--
tests/scrutiny.expected | 118 +++---
tests/specialization.expected | 18 +++--
7 files changed, 154 insertions(+), 127 deletions(-)
create mode 100644 tests/redact-gensyms.scm
diff --git a/distribution/manifest b/distribution/manifest
index c1a615e..928d5ef 100644
--- a/distribution/manifest
+++ b/distribution/manifest
@@ -176,6 +176,7 @@ tests/scrutiny-tests-2.scm
tests/scrutiny-tests-3.scm
tests/scrutiny.expected
tests/scrutiny-2.expected
+tests/redact-gensyms.scm
tests/test-scrutinizer-message-format.scm
tests/scrutinizer-message-format.expected
tests/syntax-rule-stress-test.scm
diff --git a/tests/redact-gensyms.scm b/tests/redact-gensyms.scm
new file mode 100644
index 000..31ca8a2
--- /dev/null
+++ b/tests/redact-gensyms.scm
@@ -0,0 +1,25 @@
+(module
+ redact-gensyms
+ ()
+ (import scheme)
+ (import (chicken base))
+ (import (chicken irregex))
+ (import (chicken type))
+ (import (only (chicken io) read-line)
+(only (chicken process-context) command-line-arguments)
+(only (chicken string) string-split))
+
+ (define prefixes (if (null? (command-line-arguments))
+ '("tmp" "g" "scm:")
+ (string-split (car (command-line-arguments)) ",")))
+
+ (let ((rege (irregex `(: bow ($ (or ,@prefixes)) (+ numeric)
+ (print ";; numbers replaced with XXX by redact-gensyms.scm")
+ (print ";; prefixes: " prefixes)
+ (let lp ()
+ (let ((l (read-line)))
+ (if (not (eof-object? l))
+ (begin
+(print (irregex-replace/all rege l 1 "XXX"))
+(lp))
+ )
diff --git a/tests/runtests.sh b/tests/runtests.sh
index c6f9252..7b067b7 100755
--- a/tests/runtests.sh
+++ b/tests/runtests.sh
@@ -114,28 +114,21 @@ $compile typematch-tests.scm -specialize -no-warnings
$compile scrutiny-tests.scm -analyze-only -verbose 2>scrutiny.out
$compile specialization-tests.scm -analyze-only -verbose -specialize
2>specialization.out
-
-# these are sensitive to gensym-names, so make them optional
-if test \! -f scrutiny.expected; then
-cp scrutiny.expected scrutiny.out
-fi
-if test \! -f specialization.expected; then
-cp specialization.expected specialization.out
-fi
-
$compile scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out
$compile test-scrutinizer-message-format.scm -A -verbose
2>scrutinizer-message-format.out || true
-diff $DIFF_OPTS scrutinizer-message-format.expected
scrutinizer-message-format.out
-diff $DIFF_OPTS scrutiny.expected scrutiny.out
-diff $DIFF_OPTS specialization.expected specialization.out
-
-# this is sensitive to gensym-names, s
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
On 2019-01-10 18:12, megane wrote:
> Evan Hanson writes:
> > Here's a signed-off version of the first patch in this set. I've also
> > updated the Windows test script and added the new files to the
> > distribution manifest.
> >
> > Please feel free to review and apply this one without waiting for the
> > others in megane's message. Doing it gradually is the only way we'll get
> > through all these.
>
> You'll have to redo your changes to the first patch, sorry about that.
No worries, here's the same again with a sign-off.
> Do you agree with approach I took about gensym'd variables in the second
> patch? If not, I think I'll have to come up with something else.
That's a nice idea, I think it's probably fine. But if we're going to do
that, why not wipe line numbers in all the expected files rather than
just the new one?
> diff --git a/tests/redact-gensyms.scm b/tests/redact-gensyms.scm
> new file mode 100644
> index 000..c6abb7b
> --- /dev/null
> +++ b/tests/redact-gensyms.scm
> @@ -0,0 +1,25 @@
> + (define prefixes (if (null? (command-line-arguments))
> + '("tmp" "g")
> + (string-split (car (command-line-arguments)) ",")))
> +
> + (let ((rege (irregex `(: ($ (or ,@prefixes)) (+ numeric)
This regex could be `(: bow ($ (or ,@prefixes)) (+ numeric)) so you avoid
accidentally replacing numbers in something like a variable called "dog1".
Evan
>From 76d260fab399e4cb6679e923ffdef496201d2fcd Mon Sep 17 00:00:00 2001
From: megane
Date: Mon, 19 Nov 2018 10:01:33 +0200
Subject: [PATCH] Add new test for scrutinizer message formatting
This makes it easy to see how scrutinizer changes affect the user facing
output messages.
In the test scripts, move scrutiny-tests-2.scm up so all output is
generated before diffing anything. This way you can update all expected
message files at the same time.
Signed-off-by: Evan Hanson
---
distribution/manifest | 2 +
tests/runtests.bat| 10 +-
tests/runtests.sh | 6 +-
tests/scrutinizer-message-format.expected | 261 ++
tests/test-scrutinizer-message-format.scm | 87 ++
5 files changed, 362 insertions(+), 4 deletions(-)
create mode 100644 tests/scrutinizer-message-format.expected
create mode 100644 tests/test-scrutinizer-message-format.scm
diff --git a/distribution/manifest b/distribution/manifest
index 9c1dcfdb..c1a615ea 100644
--- a/distribution/manifest
+++ b/distribution/manifest
@@ -176,6 +176,8 @@ tests/scrutiny-tests-2.scm
tests/scrutiny-tests-3.scm
tests/scrutiny.expected
tests/scrutiny-2.expected
+tests/test-scrutinizer-message-format.scm
+tests/scrutinizer-message-format.expected
tests/syntax-rule-stress-test.scm
tests/syntax-tests.scm
tests/syntax-tests-2.scm
diff --git a/tests/runtests.bat b/tests/runtests.bat
index f739a297..47d80c0c 100644
--- a/tests/runtests.bat
+++ b/tests/runtests.bat
@@ -92,19 +92,25 @@ echo scrutiny tests ...
if errorlevel 1 exit /b 1
a.out
if errorlevel 1 exit /b 1
+
%compile% scrutiny-tests.scm -A -verbose 2>scrutiny.out
if errorlevel 1 exit /b 1
rem this is sensitive to gensym-names, so make it optional
if not exist scrutiny.expected copy /Y scrutiny.out scrutiny.expected
-fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out
+%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out
if errorlevel 1 exit /b 1
+%compile% test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out
+rem this is expected to fail, so no errorlevel check
-%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out
+fc /lb%FCBUFSIZE% /w scrutinizer-message-format.expected scrutinizer-message-format.out
+if errorlevel 1 exit /b 1
+fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out
if errorlevel 1 exit /b 1
if not exist scrutiny-2.expected copy /Y scrutiny-2.out scrutiny-2.expected
+
fc /lb%FCBUFSIZE% /w scrutiny-2.expected scrutiny-2.out
if errorlevel 1 exit /b 1
diff --git a/tests/runtests.sh b/tests/runtests.sh
index 6da7630d..c6f9252e 100755
--- a/tests/runtests.sh
+++ b/tests/runtests.sh
@@ -123,11 +123,13 @@ if test \! -f specialization.expected; then
cp specialization.expected specialization.out
fi
+$compile scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out
+$compile test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out || true
+
+diff $DIFF_OPTS scrutinizer-message-format.expected scrutinizer-message-format.out
diff $DIFF_OPTS scrutiny.expected scrutiny.out
diff $DIFF_OPTS specialization.expected specialization.out
-$compile scrutiny-tests-2.scm -A 2>scrutiny-2.out -verbose
-
# this is sensitive to gensym-names, so make it optional
if test \! -f scrutiny-2.expected; then
cp scrutiny-2.expected scrutiny-2.out
diff --git a/tests/scrutinizer-message-format.expected b/tests/scrutinizer-message-format.expected
new file mode 100644
index ..f41fb898
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Evan Hanson writes: > Here's a signed-off version of the first patch in this set. I've also > updated the Windows test script and added the new files to the > distribution manifest. > > Please feel free to review and apply this one without waiting for the > others in megane's message. Doing it gradually is the only way we'll get > through all these. Could you hold on for just for a minute, please! I have a revised patch set that should be nicer for review. I'll try to post it today. Cheers ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Here's a signed-off version of the first patch in this set. I've also updated the Windows test script and added the new files to the distribution manifest. Please feel free to review and apply this one without waiting for the others in megane's message. Doing it gradually is the only way we'll get through all these. Best, Evan >From 3096aedfdf1d5ff1eea451c5e90310407b0a7ecf Mon Sep 17 00:00:00 2001 From: megane Date: Mon, 19 Nov 2018 10:01:33 +0200 Subject: [PATCH] Add new test for scrutinizer message formatting This new test covers most, but not all, scrutinizer messages. In the test scripts, scrutiny-tests-2.scm has been moved up so that all output is generated before diffing anything. Signed-off-by: Evan Hanson --- distribution/manifest | 2 + tests/runtests.bat| 10 +- tests/runtests.sh | 6 +- tests/scrutinizer-message-format.expected | 238 ++ tests/test-scrutinizer-message-format.scm | 77 ++ 5 files changed, 329 insertions(+), 4 deletions(-) create mode 100644 tests/scrutinizer-message-format.expected create mode 100644 tests/test-scrutinizer-message-format.scm diff --git a/distribution/manifest b/distribution/manifest index 9c1dcfdb..c1a615ea 100644 --- a/distribution/manifest +++ b/distribution/manifest @@ -176,6 +176,8 @@ tests/scrutiny-tests-2.scm tests/scrutiny-tests-3.scm tests/scrutiny.expected tests/scrutiny-2.expected +tests/test-scrutinizer-message-format.scm +tests/scrutinizer-message-format.expected tests/syntax-rule-stress-test.scm tests/syntax-tests.scm tests/syntax-tests-2.scm diff --git a/tests/runtests.bat b/tests/runtests.bat index f739a297..47d80c0c 100644 --- a/tests/runtests.bat +++ b/tests/runtests.bat @@ -92,19 +92,25 @@ echo scrutiny tests ... if errorlevel 1 exit /b 1 a.out if errorlevel 1 exit /b 1 + %compile% scrutiny-tests.scm -A -verbose 2>scrutiny.out if errorlevel 1 exit /b 1 rem this is sensitive to gensym-names, so make it optional if not exist scrutiny.expected copy /Y scrutiny.out scrutiny.expected -fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out +%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out if errorlevel 1 exit /b 1 +%compile% test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out +rem this is expected to fail, so no errorlevel check -%compile% scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out +fc /lb%FCBUFSIZE% /w scrutinizer-message-format.expected scrutinizer-message-format.out +if errorlevel 1 exit /b 1 +fc /lb%FCBUFSIZE% /w scrutiny.expected scrutiny.out if errorlevel 1 exit /b 1 if not exist scrutiny-2.expected copy /Y scrutiny-2.out scrutiny-2.expected + fc /lb%FCBUFSIZE% /w scrutiny-2.expected scrutiny-2.out if errorlevel 1 exit /b 1 diff --git a/tests/runtests.sh b/tests/runtests.sh index 6da7630d..c6f9252e 100755 --- a/tests/runtests.sh +++ b/tests/runtests.sh @@ -123,11 +123,13 @@ if test \! -f specialization.expected; then cp specialization.expected specialization.out fi +$compile scrutiny-tests-2.scm -A -verbose 2>scrutiny-2.out +$compile test-scrutinizer-message-format.scm -A -verbose 2>scrutinizer-message-format.out || true + +diff $DIFF_OPTS scrutinizer-message-format.expected scrutinizer-message-format.out diff $DIFF_OPTS scrutiny.expected scrutiny.out diff $DIFF_OPTS specialization.expected specialization.out -$compile scrutiny-tests-2.scm -A 2>scrutiny-2.out -verbose - # this is sensitive to gensym-names, so make it optional if test \! -f scrutiny-2.expected; then cp scrutiny-2.expected scrutiny-2.out diff --git a/tests/scrutinizer-message-format.expected b/tests/scrutinizer-message-format.expected new file mode 100644 index ..9c7299f7 --- /dev/null +++ b/tests/scrutinizer-message-format.expected @@ -0,0 +1,238 @@ + +Warning: literal in operator position: (1 2) + +Warning: literal in operator position: (1 2) + +Warning: in toplevel procedure `r-proc-call-argument-count-mismatch': + (test-scrutinizer-message-format.scm:9) in procedure call to `scheme#cons', expected 2 arguments but was given 1 argument + +Warning: in toplevel procedure `r-proc-call-argument-type-mismatch': + (test-scrutinizer-message-format.scm:10) in procedure call to `scheme#length', expected argument #1 of type `list' but was given an argument of type `symbol' + +Warning: in toplevel procedure `r-proc-call-argument-value-count': + (test-scrutinizer-message-format.scm:11) expected a single result in argument #1 of procedure call `(scheme#list (chicken.time#cpu-time))', but received 2 results + +Warning: in toplevel procedure `r-proc-call-argument-value-count': + (test-scrutinizer-message-format.scm:11) expected a single result in argument #1 of procedure call `(scheme#vector (scheme#values))', but received zero results + +Warning: in toplevel procedure `r-proc-call-argument-value-count': + expected a single result
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Heads up. This needs to be updated as 0011 is now in the master branch. I'll update both of these batches and try to write better commit messages for them too. ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> > Here's the patchset without the first commit. > Thanks, much appreciated. Now we just have to find someone who understands the scrutinizer and can review these patches... :-) felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
On Mon, Nov 19, 2018 at 08:16:45PM +0100, felix.winkelm...@bevuta.com wrote: > Be that as it may, I don't think such changes have any benefit. We don't > follow a beauty contest, content is more important as the amount of > whitespace, > comment or indentation style and so on. Changes of functionality may include > such > superficial modifications but once we start with purely syntactic > modifications > for their own sake, we will end up with style guides, which is something I > don't want. FWIW, I totally agree. I can't stand style guides. Having an informal set of agreements to make the code look consistent is enough, and it's fine to tweak layout to be more consistent while you're changing the code in that area (like our rule^Wtradition to replace [] with () when such code is touched), but let's not start stricly enforcing anything. I have to deal with enough of these sort of bullshit rules at work, and I want CHICKEN to remain fun to hack on without getting slapped on the wrist about arbitrary rule "violations" decided upon by a stupid machine or overzealous people. I know I sometimes am guilty of doing exactly that, so feel free to correct me when I do so! Having said that, I do like our .dir-locals.el setup to have at least somewhat consistent auto-indenting. That's nonintrusive and just a matter of configuring your editor once (or not at all when using Emacs). Cheers, Peter signature.asc Description: PGP signature ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> > I'm vehemently against patches that merely change whitespace or
> > layout (patch 1/9) and make it impossible to track changes properly.
>
> You can give the -w switch to git blame to see through whitespace
> changes. Or (setq vc-git-annotate-switches '("-w")) in emacs if you're
> using vc-annotate, which I'd suggest anyone to try.
Be that as it may, I don't think such changes have any benefit. We don't
follow a beauty contest, content is more important as the amount of whitespace,
comment or indentation style and so on. Changes of functionality may include
such
superficial modifications but once we start with purely syntactic modifications
for their own sake, we will end up with style guides, which is something I
don't want.
felix
___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
felix.winkelm...@bevuta.com writes:
>>
>> Hi,
>>
>> Here's a reworked patch set. It's not exactly small, but I tried to make
>> it pretty easy to follow. Except maybe for the last patch, which
>> digs for some extra info from the nodes.
>>
>> There's small bit of back-and-forth in the patches:
>> - errors? is taken out of let and put back
>> - report-notice lingers unused before getting deleted
>>
>> There were some whitespace warnings when I applied this, but everything
>> seemed to work fine.
>
> I'm vehemently against patches that merely change whitespace or
> layout (patch 1/9) and make it impossible to track changes properly.
You can give the -w switch to git blame to see through whitespace
changes. Or (setq vc-git-annotate-switches '("-w")) in emacs if you're
using vc-annotate, which I'd suggest anyone to try.
>
>
> felix
___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
> > Hi, > > Here's a reworked patch set. It's not exactly small, but I tried to make > it pretty easy to follow. Except maybe for the last patch, which > digs for some extra info from the nodes. > > There's small bit of back-and-forth in the patches: > - errors? is taken out of let and put back > - report-notice lingers unused before getting deleted > > There were some whitespace warnings when I applied this, but everything > seemed to work fine. I'm vehemently against patches that merely change whitespace or layout (patch 1/9) and make it impossible to track changes properly. felix ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
[Chicken-hackers] [PATCH] Use vertical space more liberally in some scrutinizer messages
Hello, This patch changes the warning messages so that any types are printed with `pp` on new lines. This should make it just a bit easier to find the reason for more complex type mismatches. See the attachment for sample output. diff --git a/scrutinizer.scm b/scrutinizer.scm index ece07ed..07de124 100644 --- a/scrutinizer.scm +++ b/scrutinizer.scm @@ -144,11 +144,27 @@ (define-inline (value-type? t) (or (struct-type? t) (memq t value-types))) -(define (type-name x) - (let ((t (strip-syntax x))) -(if (refinement-type? t) - (sprintf "~a-~a" (string-intersperse (map conc (second t)) "/") (third t)) - (sprintf "~a" t +(define (string-add-indent str #!optional (ind " ")) + (let* ([ls (string-split str "\n")] + [s (string-intersperse + (map (lambda (l) (if (string=? "" l) l + (string-append ind l))) + ls) + "\n")]) +(if (eq? #\newline (string-ref str (sub1 (string-length str + (string-append s "\n") + s))) + +(define (type->pp-string t) + (string-add-indent + (string-chomp +(with-output-to-string + (lambda () + (let ([t (strip-syntax t)]) + (if (refinement-type? t) + (printf "~a-~a" (string-intersperse (map conc (second t)) "/") (third t)) + (pp t)) + "")) (define specialization-statistics '()) (define trail '()) @@ -276,15 +292,14 @@ (define (always-true if-node test-node t loc) (and-let* ((_ (always-true1 t))) (report-notice - loc "~aexpected a value of type boolean in conditional, but \ - was given a value of type `~a' which is always true:~%~%~a" - (node-source-prefix test-node) t (pp-fragment if-node)) + loc "~aIn conditional~%~%~a~%~% the test expression has type~%~%~a~%~% which is always true." + (node-source-prefix test-node) (pp-fragment if-node "") (type->pp-string t)) #t)) (define (always-false if-node test-node t loc) (and-let* ((_ (eq? t 'false))) (report-notice - loc "~ain conditional, test expression will always return false:~%~%~a" + loc "~aIn conditional, test expression will always return false:~%~%~a" (node-source-prefix test-node) (pp-fragment if-node)) #t)) @@ -315,13 +330,13 @@ (when complain (##sys#notice (conc (location-name loc) - (sprintf "~?" msg (map type-name args)) + (sprintf "~?" msg args) (define (report loc msg . args) (when complain (warning (conc (location-name loc) - (sprintf "~?" msg (map type-name args)) + (sprintf "~?" msg args) (define (report-error loc msg . args) (set! errors #t) @@ -342,11 +357,13 @@ (map (cute walk <> (add1 d)) xs))) (else (strip-syntax x)) -(define (pp-fragment x) - (string-chomp - (with-output-to-string - (lambda () - (pp (fragment x)) +(define (pp-fragment x #!optional (ind " ")) + (string-add-indent + (string-chomp + (with-output-to-string + (lambda () + (pp (fragment x) + ind)) (define (get-specializations name) (let* ((a (variable-mark name '##compiler#local-specializations)) @@ -355,9 +372,10 @@ (and (pair? c) c))) (define (call-result node args e loc params typeenv) - (define (pname) - (sprintf "~ain procedure call to `~s', " + (define (pname #!optional (pred "")) + (sprintf "~aProcedure call to ~a`~s' " (node-source-prefix node) + pred (fragment (first (node-subexpressions node) (let* ((actualtypes (map walked-result args)) (ptype (car actualtypes)) @@ -370,10 +388,10 @@ (cond ((and (not pptype?) (not (match-types xptype ptype typeenv))) (report loc - "~aexpected a value of type `~a' but was given a value of type `~a'" + "~aexpected a value of type~%~%~a~%~% but was given a value of type~%~%~a" (pname) - (resolve xptype typeenv) - (resolve ptype typeenv)) + (type->pp-string (resolve xptype typeenv)) + (type->pp-string (resolve ptype typeenv))) (values '* #f)) (else (let-values (((atypes values-rest ok alen) @@ -395,11 +413,11 @@ typeenv) (report loc - "~aexpected argument #~a of type `~a' but was given an argument of type `~a'" + "~aexpected argument #~a of type~%~%~a~%~% but was given an argument of type~%~%~a" (pname) i - (resolve (car atypes) typeenv) - (resolve (car actualtypes) typeenv + (type->pp-string (resolve (car atypes) typeenv)) + (type->pp-string (resolve (car actualtypes) typeenv) (when (noreturn-procedure-type? ptype) (set! noreturn #t)) (let ((r (procedure-result-types ptype values-rest (cdr actualtypes) typeenv))) @@ -412,9 +430,8 @@ (cond ((match-argument-types (list pt) (cdr actualtypes) typeenv) (report-notice loc - "~athe predicate is called with an argument of type `~a' \ - and will always return true" - (pname) (cadr actualtypes)) + "~awith an
