New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix built-in syntax-rules (compare, rename, etc.) #330

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Hamayama
Contributor

Hamayama commented Dec 21, 2017

#329 とは違うかもしれませんが、literals の判定で問題が出るテストケースを見つけました。

compare-literals-2 というテストで、組み込みの syntax-rules では結果が failed になります。

これは、chibi, sagittarius, racket では、展開エラーになります。

それで、いろいろ調べて変更していくうちに、compare と rename について見直した感じになりました。

(主に srfi-149 参照実装の最新版を参考にしました。
https://github.com/ashinn/chibi-scheme/blob/master/lib/init-7.scm#L687
(srfi-149 文書のものよりも更新されています))

<変更内容>
src/macro.c
src/compile.scm
test/macro.scm

  1. 比較処理の変更
    (Sym-or-id, Sym-or-id) -> Bool の比較を行うために compare 関数を作成して、
    中で compile.scm の er-comparer を呼び出すようにした。
    (このとき、er-comparer の引数が uenv になっていて、うまく呼び出せなかったため、
    er-comparer の引数を module と env に変更しました)
    そして、compare_ellipsis と match_identifier を、compare に置き換えた。
    (対応テストケース : compare-literals-2)

  2. rename の変更
    compile_rule1 で、literals と others のみ rename するようにした。
    (pattern variables は、pattern と templete の比較(Scm_Assq)
    にのみ使われるため rename は不要と思われる)
    また、compile_rule1 で、literals の検出には Scm_Memq を使うようにした(srfi-149 より)。
    また、preprocess_literals では rename をやめた。
    (対応テストケース : compare-literals-2)

  3. add_pvar で、level と count の MAX チェックを追加
    (MAX を超えると変な状態になったため)
    (対応テストケース : pattern-variables-1)

  4. compile_rules で、ctx.ellipsis = FALSE;ctx.ellipsis = SCM_FALSE; に変更
    (compare で SEGV エラーになったため)
    (対応テストケース : alt-elli3)

  5. compile_rule1 で、underbar の比較を compare を使うように変更
    (マクロで underbar を生成したときに機能しなかったため)
    (対応テストケース : gen-underbar-1)

<テスト>
OS : Windows 8.1 (64bit)
Gauche : コミット 8884cf7 + 変更
開発環境 : MSYS2/MinGW-w64 (64bit) (gcc version 7.1.0 (Rev2, Built by MSYS2 project))
Total: 19273 tests, 19273 passed, 0 failed, 0 aborted.

pattern-match-lambda の test.scm - OK

Ypsilon の syntax-rules stress test - OK

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 22, 2017

macro.c の match_identifier を削除しました。

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 22, 2017

r7rs-tests.scm の underscore のケースが通るようになったので、有効にしました。

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 22, 2017

どうも、本当に必要な wrap は、realize_template_rec のところのものであり、
compile_rule1 における wrap(=rename) は、realize_template_rec に、
「マクロ定義時の環境 および wrapが必要であるという情報」を届ける役割を果たしている
ということが分かってきました。

すなわち、compile_rule1 では、x のかわりに (rename 'x) を埋め込んでいる、みたいな。。。

現状、これで正常に動作していますが、2重に wrap しているため、
展開結果に以下のような無駄ができます。
#<identifier user##<identifier user#temp.2d9c6e0>.2db8600>

これを例えば、realize_template_rec の Scm_WrapIdentifier の下に
SCM_IDENTIFIER(id)->name = SCM_IDENTIFIER(template)->name;
という行を追加して、wrap を一段削除するようにすると、展開結果は
#<identifier user#temp.2d9c6e0>
のようになり、これでもテストは全て通ります。

ただ、このようなことをすると、もし、compile_rule1 の wrap に抜けがあった場合に、
重要な情報が削除されてしまい、ものすごく分かりにくい不具合が出そうです。

compile_rule1 で wrap(=rename) に加えて何か目印を埋め込むようなことも
考えたのですが、あまり良い方法が思いつきませんでした。

現状だとしかたがない感じでしょうか。。。

@shirok

This comment has been minimized.

Owner

shirok commented Dec 22, 2017

ども。ご指摘のとおり、compile_rule1でのrenameは重複していて、本当に必要なのはerマクロと同じく、compile時点での環境を補足したrenameクロージャとcompareクロージャを作ることです。実際のrenameは展開時にやれば良いので。
ただ、今のCで書いたsyntax-rules展開器はいずれ書き換えるので、当面のfixはあまり綺麗にすることを考えなくて良いと思います。

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 23, 2017

identifier の構造体に flags というメンバーを追加して、
マクロ展開時に wrap を一段削除するようにしてみました。

同様に、identifier の frames の短縮の遅延についても、
flags を使うようにしてみました。

あまりよくない考えかもしれませんが、一案ということで。。。

しかし C のマクロ展開器はよくできていますね。
このままでよいような気もしますが。。。

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 27, 2017

少しだけ変更しました。

あと、処理時間を測定してみたのですが、若干遅くなっていました。

以下は、Ypsilon の syntax-rules stress test の結果です。

コミット 5159911 : real 0.54 user 0.00 sys 0.01
本変更           : real 0.76 user 0.00 sys 0.03
srfi-149-mod     : real 3.28 user 0.00 sys 0.00

(それでも srfi-149-mod よりは 4 倍以上速いですが。。。)

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 28, 2017

ellipsis のエラーケースが見つかったので対応しました。
(このテストは展開エラーになるのが正しくて、
現状の Gauche でもエラーになりますが、
本変更の影響で、エラーが出なくなっていました)

  1. ellipsis を指定したときは、SCM_EQ で判定 (literalと同様)
    ellipsis を指定しなかったときは、compare で判定 (underbarと同様)

    ellipsis の指定と未指定を区別するため、compile-1.scm で、
    未指定時には elli に #t をセットするようにしました。
    (#f は、すでにellipsis 無効時に使用していたため)

    (対応テストケース : compare-ellipsis-1, compare-ellipsis-2)

@shirok

This comment has been minimized.

Owner

shirok commented Jan 3, 2018

ども。概ねこのまま頂きますが、私自身が後で理解しやすいように、fix毎のパッチに切り分けてコミットしようと思います。

shirok added a commit that referenced this pull request Jan 6, 2018

shirok added a commit that referenced this pull request Jan 6, 2018

@shirok

This comment has been minimized.

Owner

shirok commented Jan 6, 2018

identifierにflagを付加する部分以外を 756de387e25a92 で頂きました。

identifierのflagなんですが、set/reset操作のMT safetyが自明でない (読み出し→論理演算→書き込みの間に別のフラグ操作が入るとraceが起きる)ので保留にします。identifierのライフサイクル的に操作が重なることは無いとは思うんですが、条件をはっきりさせておかないと将来拡張したときにうっかり罠を踏んじゃう可能性があるので。

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Jan 7, 2018

フラグの件了解です。
(スレッドのことを考えていませんでした。。。)

macro.c ですが、差分を確認して3点気になったところがあります。
(動作には問題ありません)

(1) PatternContext 構造体のメンバー ellipsis の定義のところのコメントで、
SCM_TRUE をデフォルト用に使っていることを書いておいてほしい気がする。

(2) preprocess_literals 関数のコメントで、
/* convert literal symbols into identifiers */
とあるが、現在は、identifier への変換は行っていない。

(3) match_identifier 関数が未使用になったので削除可能

以上です。

@shirok

This comment has been minimized.

Owner

shirok commented Jan 7, 2018

確認ありがとうございます。

shirok added a commit that referenced this pull request Jan 10, 2018

@shirok

This comment has been minimized.

Owner

shirok commented Jan 10, 2018

上の3点、取り入れました。794c18f

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Jan 10, 2018

ありがとうございます。クローズします。

#329 も、クローズしてよいかと思います。

現状をまとめると、以下のようになっています。

  • コンパイル時の比較方法 (compile_rule1)

    項目 比較方法
    ellipsis(指定あり) eq?
    ellipsis(指定なし) compare
    literal(指定あり) eq?
    underbar compare
    pattern variable eq?
  • 使用時の比較方法 (match_synrule)

    項目 比較方法
    pattern variable
    underbar
    identifier compare
    literal(identifier以外) equal?

    △:コンパイル時に場所が分かっているので、場所に応じて処理

  • 出力時のrename (realize_template_rec)

    項目 rename
    pattern variable
    identifier
    literal(identifier以外)

    ○:renameあり
    -:renameなし

コンパイル時の比較方法については、
syntax-rules 内で明示的に指定されている識別子については eq? を使用しており、
そうでないものについては compare を使用しています。

使用時の比較方法については、identifier については compare を使用しており、
それ以外の literal については equal? を使用しています。

出力時のrenameについては、identifier についてのみ rename しており、
それ以外の literal および pattern variable の内容については、そのまま出力しています。

@Hamayama Hamayama closed this Jan 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment