Menu

#1115 追加のコマンドラインオプションを既存プロセスに渡します。

open
nobody
None
5
2018-04-19
2018-04-03
ds14050
No

掲示板の件です。
http://sakura-editor.sourceforge.net/cgi-bin/cyclamen/cyclamen.cgi?log=data&v=8322#8322

複数のファイルが与えられたときに最初のファイルにしかマクロオプションが渡らないなど、完成度が足りません。

これまで通りのキャレット位置と、要望のあったマクロオプションだけを解釈していますが、その他のオプションに追加で対応するのは容易です。

1 Attachments

Discussion

1 2 > >> (Page 1 of 2)
  • ds14050

    ds14050 - 2018-04-06

    パッチの第2版です。

    • マクロオプション(-M, -MTYPE)が2番目以降のファイルにも渡せるようになりました。
    • それを実行する CNormalProcess::OpenFiles がインスタンスに依存しない内容だったので、 .cpp ファイルの中に隠しました。
    • 実効性のない注意喚起でしかなかった global.h への変更を取り消しました。

    空文字列("")を含むマクロ(InsText("")など)が実行時エラーになりますが、これはマクロオプションを読み込むときに以前からあった問題のようです。オプションが二重引用符で囲まれていようといなかろうと "" を " へと置換しています。

    >sakura a.txt a.txt a.txt -M=InsText("A") -MTYPE=js
    

    自然な帰結として上記のように同じファイル名を繰り返すことでマクロが繰り返し実行されます。面白いので余計なことはせずにそのままです。

     

    Last edit: ds14050 2018-04-06
    • ds14050

      ds14050 - 2018-04-06

      一時変数が適切なスコープで始末されるように、CLQuote関数を関数オブジェクト化したうえで OpenFiles関数のローカル変数としました。

       
  • berryzplus

    berryzplus - 2018-04-07

    ds14050 さん

    パッチ作成お疲れ様です。コード読ませていただきました。
    掲示板のほうに「期待通り」とのコメントがあり、文句なしの出来だと思います。

    --少し別のお話です--

    sakuraの中でコマンドラインを扱う処理は大きく分けて3つあると思っています。
    1. 起動時に自プロセス(sakura.exe)のオプションを取得する処理
    2. 新しいsakura.exeを起動する処理
    3. 外部ツール(sakura.exe以外)を起動する処理

    この修正パッチは、
    4. 起動済みのsakura.exeにオプションを渡す処理
    を追加します。

    既存コードはオブジェクト志向ではありませんが、
    概念的なオブジェクト階層は下のようになります。

    汎用コマンドライン ← 3で利用するもの(実在しない)
     ┗ sakura専用コマンドライン ← 1, 2, 4で利用するもの(実際2では利用していない)

    修正パッチでは、4のために新規オブジェクトを追加しているように見えます。
    std::auto_ptr<CCommandLine>みたいなアプローチができれば
    新規オブジェクトはいらないのですが、今回は追加をしています。

    2と3の処理がコマンドラインオブジェクトを使ってない現状で、
    こういうオブジェクト指向的な整理を考えるのは不毛かも知れませんが、
    いまできることはやっておくほうがよいように思います。
    CAnotherCommandLineの追加は、適切でしょうか?

    いまsakuraのプロジェクトは殆ど動いていない状態です。
    個人的には、このまま自然消滅するんじゃないかと思っています。
    開発者のモチベーションが保てないのだから仕方ありません。
    ぼくはsakuraが好きなので完全に消してしまうのは嫌です。
    消えてしまわないように、週1くらいで掲示板を見ていますし、
    某サイトに派生プロジェクトを起ち上げたりしています。

    もし自然消滅せずに活動が継続された場合、
    いつかは既存コードの整理が行われると思っています。
    コード整理で消えていくコードと、残っていくコードの違いはなんでしょうか。
    整理する人の趣味にあうかどうか、ってことだけじゃないとぼくは思うんですよね。

     
    • ds14050

      ds14050 - 2018-04-07

      そうですね。死に体ですね。反応があるのは嬉しいものです。

      CAnotherCommandLine について言えば、これは自分の趣味や理想などではなく、CCommandLine の持つ解析ルーチン、アクセサを使いたい、でもシングルトンだから使えないというところから発生した、最も簡便な(diffの少ない)解決策です。継承の主と従が入れ違っているように見える命名であるために、やや長めのコメントを入れる必要を感じました。

      3で利用する可能性があるという汎用コマンドラインの想定が抜けた状態で CCommandLine から発想して CAnother~と命名しましたが、そう考えると CSakuraCommandLine が適切に思えます。しかしそこまでです。

      CAnotherCommandLineは新しいオブジェクトでしょうか。新しいオブジェクト、新しいコードを書かないために CAnotherCommandLineとそのシングルトンバージョン(CCommandLine)の継承関係が存在しています。どちらも「サクラエディタのコマンドライン」を表すものですが、あるサクラエディタと唯一の存在である自分自身とを区別するものとしてシングルトン性が付与されています、というか付与されていました。

      こういう特殊化の階層が考えられ、一番下の一番特別なものが最初から存在したものです。

      • 汎用コマンドライン
        • サクラエディタを起動するコマンドライン
          • 自分自身を起動したコマンドライン

      シングルトン性を剥奪したコマンドラインクラスに一本化できますか。今ある CCommandLineに相当するものをどこに置き、どうアクセスしましょうか。TSingletonがその手段のひとつで、現在採用されているものではないですか。

      顔を真っ赤にして反論をタイプする姿が鏡の中に見えるようです。この長文ではしかたないでしょう。一度考えをめぐらせた問題について別の視点からの、あるいはより優れた答えを得るというのは、またとない学習機会になるものです。一読素通りした内容も穴にはまってから読み返せばその身につき方が違います。しかし他にあるベストな手段はまだ見えていません。あのパッチが精一杯です。

       

      Last edit: ds14050 2018-04-07
  • berryzplus

    berryzplus - 2018-04-08

    ds14050さん

    現状の実装は既に要求を満たしていると思います。
    なので、突っ込みをいれるかどうか迷いました。

    絶対的な回答のある話題でもないので、
    あくまで一意見としてとらえてください。

    CAnotherCommandLine について言えば、これは自分の趣味や理想などではなく、
    CCommandLine の持つ解析ルーチン、アクセサを使いたい、
    でもシングルトンだから使えないというところから発生した、
    最も簡便な(diffの少ない)解決策です。

    流用したいクラスがあるけどシングルトンだから使えない。
     ↓
    シングルトンを外したクラスを作ろう。

    この点は、ぼくがやるとしても同じです。
    ただ、「シングルトンを外そう」のあとに追加でいくつか考えて、
    既存クラスが担っていた役割の方を別名で新設する気がします。

    1, 元のクラスCCommandLineの役割は何か?
     →sakuraの起動オプションへのアクセスを提供する
    2, 元のクラスは何故、シングルトンなのか?
     →プロセスを起動したコマンドラインは唯一無二のものだから。
    3, 新しい用途と従来用途の違いは何か?
     →新しい用途では、起動した後に何度も発生する。(シングルトンじゃマズい)
    4, 何故、最も簡便な(diffの少ない)解決策を取りたいのか?
     →いつ、誰が、何を変更したのか分かりやすくするため。

    修正パッチをあてた状態のコードを見ると、
    CAnotherCommandLineというクラスをaroka氏が書いたように見えます。
    (これはぼくの感想なので、違う見方もあるかも知れません。)

    理想で書いた汎用コマンドラインについて、説明が足りてなかったので追記します。

    汎用コマンドライン(読み書き可能、外部ツールを起動するために使う。)
    ┗sakura専用コマンドライン(読み書き可能、sakura.exeを起動するために使う。)
     ┗自プロセス起動コマンドライン(読み取り専用、オプションを取得するために使う。)

    存在しないものの話をするのに、用途の説明をしていなかったのが大ポカでした。

     
  • ds14050

    ds14050 - 2018-04-09

    既存クラスが担っていた役割の方を別名で新設する気がします。

    焦点はここなのでしょうか。前回のコメントでは新規クラスの追加を嫌われたと思いました。

    名前について悩んでそれが長めのコメントにつながったのはすでに書いた通りです。CAnotherCommandLine に自然な名前(たとえば CCommandLine)を付けて CCommandLine を CMyCommandLine のような名前にするのが最初の思いつきでした。

    「絶対的な回答のある話題でもない」というのはその通りだと思います。自分は、名前とそれが表すものの結びつきを維持する選択をしました。サクラエディタはオープンソースであり自分でパッチを当てる人がいます。CCommandLine という名前がなくなったり指す対象が変わってしまったりすれば、その部分に手を入れていた人は本家の変更を追跡し理解しパッチを書き直す手間が生じます。

    CAnotherCommandLineというクラスをaroka氏が書いたように見えます。

    名前は変わりましたが中身はそうです。自分がそのクラスを書いたと言えないのは間違いのないところです。

    問題があるならコメントやバージョン履歴でフォローできると思います。


    汎用コマンドラインのことですが、汎用コマンドラインが外部コマンド起動専用であれ、本当の汎用であり、外部コマンド起動専用コマンドラインがサクラエディタ起動コマンドラインの兄弟としてぶら下がるとしても、議論の何に影響するかがわかりません。

     
  • berryzplus

    berryzplus - 2018-04-13

    既存クラスが担っていた役割の方を別名で新設する気がします。

    焦点はここなのでしょうか。前回のコメントでは新規クラスの追加を嫌われたと思いました。

    読み返しましたが新規クラスがダメとは書いてませんね。
    焦点はCAnotherCommandLineクラスが適切かどうかです。

    適切じゃないような気がしていて、
    何故こうなったのか、とか、
    逆にどうだったら適切といえそうか、とか
    そうことについて書いてきたつもりです。

    CCommandLine という名前がなくなったり指す対象が変わってしまったりすれば、
    その部分に手を入れていた人は本家の変更を追跡し理解しパッチを書き直す手間が生じます。

    そういう人がいるなら、呼んできて手伝ってもらいませんか。

    いるかどうかも分からない、プロジェクトが困っていても助けてくれない、
    そんな人達のために本家の開発者が縛られるのは理不尽です。
    オープンソースの開発者はもっと自由であるべきです。

    サクラエディタがオープンソースと知りながら、
    自己流で加えた改善の成果を報告せず、
    似たような問題に直面した有志プログラマの時間を浪費させてきたんです。
    ある意味「万死に値する」と思うんですがどうでしょう。

    名前について悩んでそれが長めのコメントにつながったのはすでに書いた通りです。

    プログラミングの格言でこういうのがあります。
    Don't comment bad code, rewrite it.
    意訳:コメントで誤魔化そうと頑張るよりも、書き直してしまったほうが早い

    格言を鵜呑みにすれば、説明コメントが必要と感じた時点で
    これはダメな作戦かもしれないと気づいたことになります。

    説明が必要なら書き直せ、という言葉に従えば、
    CCommandLine自体も、もともと良い名前でないのかも知れませんね。

     
    • ds14050

      ds14050 - 2018-04-14

      現状の問題認識はこうです。

      • CAnotherCommandLine という名前は不適切なので CSakuraCommandLine に改める。

      以上です。独自のパッチを抱えているうちの一人は自分です。原則と現実にずれはあります。すべてが本家に還元できるものではありません。

      「万死に値する」という意見には与しません。協力するもしないも自由です。ただ、パッチの取り込みを働きかけないと本家の変更をマージする自分の作業がつらくなるというのはあるでしょう。それを自業自得と考えてあえてこちらで CCommandLine という名前の一貫性を考慮する必要などないというのもひとつの考えです。

      ここからが絶対的な答えのない部分ですが、あえて手を加えてまで一貫性を維持しようとは自分も考えませんが、手を加えなければ壊れないものを独善的とも言える美意識にのみ基づいて変えることも自分はしません。保守的なんです。そんな自分がそこまでやるかと半ば呆れてしまう取り組みが『APIデザインの極意 Java/NetBeansアーキテクト探究ノート』には書かれています。それを見て自分はそこまで極端ではないなと考えるわけです。

      Don't comment bad code, rewrite it.

      知りませんでした。なかなか考えさせられる言葉です。結局のところコメントはコメントなんですよね。つい最近自分の日記にこう書きました。「コメントやドキュメントで注意喚起をしても無駄」「コードの表現力が足りていないのをコメントで補おうとするのが間違い」 どうにも忘れがちです。

      いい言葉ですが文脈が必要です。同じ本だったと思いますが、API のリリース 1.0 までになされる判断とそれ以後のものとで、基準に線引きがされていました。自分の判断は公開後の基準にやや寄っています。

      CCommandLine自体も、もともと良い名前でないのかも知れませんね。

      これは後知恵でしょう。汎用コマンドラインが不定冠詞の a commandline だとすれば CCommandLine は定冠詞の the commandline のつもりで名付けられたと考えることができます。この the は「このコマンドラインがどういうコマンドラインかは言わなくてもわかるな、の the」です(本当かよ)。他に区別すべきコマンドラインがなかったのだからしかたないでしょう。

      パッチを当てたあとはそうではないというのが berryzplus さんの指摘ですね。その事の軽重のとらえ方が意見の分かれるところで、あえて変える必要があるのかどうか。あと一人関係者の意見があればどちらであれ多数派が形成されるのですけどね。そもそも人がいないのに火中の栗を拾う人間がはたして……。

      receive_another_commandline.r2.2.patch (第2版の改良版) について

      • CAnotherCommandLine を CSakuraCommandLine に改名した。
      • CSakuraCommandLine は継承されることがわかっているので virtual デコンストラクタをつけた。

      実はシングルトンの CCommandLine を廃止して CSakuraCommandLine の static メンバ経由で特別なインスタンスにアクセスする手もあるな、だったらクラスが1つにまとまるからクラス名は CCommandLine のままでいいなと思いついたのですが、getInstance に代わる適切なメンバ名が思いつかなかったのでした。おそらく CommandLine という名前で CProcess のメンバになるのが妥当な収まりどころでしょうが、だったらそれは別の仕事です。なんとなくですがシングルトンはアプリケーションにつき1つあれば十分な気がしています。少なくとも CCommandLine にはその必要がなかった。

      そうしてみると ParseCommandLine というコンストラクタとは別に用意されたイニシャライザ、リセット関数が必要とされるのも、CCommandLine が無駄にシングルトンであったせいで適切なタイミング、適切なパラメータでの初期化ができなかったせいに思えてきます。書き直したい(だが別の仕事)。

      遡るほどにけったいな実装になっていく CCommandLine よ(笑)

       

      Last edit: ds14050 2018-04-14
      • ds14050

        ds14050 - 2018-04-14

        説明のできない違和感の正体が「デ『コン』ストラクタ」だとわかりました。デストラクタとデコンストラクトはどちらも専門用語みたいですね。

         
      • ds14050

        ds14050 - 2018-04-14
        LPCTSTR m_lpCmdLine; // ポインタが指す先の寿命は不明! 不安ならコピーを保持すること。
        

        CNormalProcess クラスにこういうメンバを追加しようとしている人間がどの口で「おそらく CommandLine という名前で CProcess のメンバになるのが妥当な収まりどころでしょうが、だったらそれは別の仕事です」なんて言うんでしょうね。ちなみに [r215] まで遡ると CProcess が LPSTR m_CommandLine; というメンバを持っていました。そのときのアクセス修飾子は protected で、派生クラス(CNormalProcess, CControlProcess)からの利用が想定されていたのですよね。それが private になってしまっていたから [r2357]において「未使用変数削除」という名目で消されてしまったのでしょう。アクセス制御の変更は [r1175] です。

        CProcess も無関係ではないみたいなのでパッチのさらなるアップデートを考えます。

         

        Related

        Commit: [r1175]
        Commit: [r215]
        Commit: [r2357]


        Last edit: ds14050 2018-04-14
  • ds14050

    ds14050 - 2018-04-18

    自重しないとこういう誰も喜ばない自己満足物質が生み出されます。

    パッチの注釈。些細でない部分のみ。分量があっても些細な変更があります。

    _main/CCommandLine.cpp

    • 一時文字列変数と strlen系の使用を減らすために関数引数型変更。szPath を使い回し。
      引数に書き込む my_strtok に代わり、書き込まない my_cstrtok を作成し使用。
      トークンの NULターミネイトが不可能なためコマンドラインオプションの
      DEBUG_TRACE をやむなくコメントアウト。
    • オプションの二重引用符の取り扱いを CheckCommandLine に一任。
    • CCommandLine::ParseCommandLine を廃止しコンストラクタに機能を統合。
      つまるところコンストラクタはインスタンスのアイデンティティ確立の場
      であり、それにとって代わるリセット関数は特別な事情がなければ不要。
    • ParseCommandLine から呼んでいた GetModuleFileName API を使用中止。
      代わりに実行ファイル名を含んだコマンドライン文字列をコンストラクタで
      受け取り、すべてをこの引数に基づいて決定する。
    • コマンドライン文字列形式の変更は、WinMain.cpp における MINGW32 に対応する
      条件付きコンパイルを取り除き、コードを共通化する過程で行われた。
    • レスポンスファイルの処理に疑問。以下のようにコメントした。
      「レスポンスファイルの内容の最初がパスだった時に、レスポンスファイルより前に
      処理された最初のファイル引数が上書きされていた。やめたけど OK?」
      「レスポンスファイルの内容が split(/\n|\r\n?/).join("") されていた。
      スペースで連結したけど OK?」
      どちらも test_cl.bat に実例を用意した。

    _main/CCommandLine.h

    • CCommandLine::getInstance という名前は CCommandLine がシングルトンではない
      今は適当でないが、これを取り除くと広範囲に些細な変更がまき散らされ差分の焦
      点がぼやけるので暫定留置。

    _main/CNormalProcess.cpp

    • 「言語を選択する」とコメントされた部分に書いたコメントを抜粋する。
      「CControlProcess ではこの位置に GetShareData().RefreshString(); があるけど?」共有メモリの更新だから誰かが1回やればいいのかな。
    • GREP やマクロ実行など、InitializeProcess が完了するまでに多くのことをやりすぎ
      ているように感じたので、MainWindow を作成するまで、SetMainWindow を呼び出す
      までを InitializeProcess の目的と再定義し、MainWindow の HWND をその戻り値
      とした。SetMainWindow は廃止。InitializeProcess で行っていた後半部分は
      MainLoop へ移動。AfterInitializeProcess メンバを新規導入する意味はなさそう。
    • オプションのヘルプに「-M=|(略)。Grep、アウトプットでは実行されません。(略)」
      とあるが、アウトプットウィンドウでは実行されている。(ver.2.3.2.0)

    _main/CProcess.cpp

    • 共有データ構造体の初期化を InitializeProcess の仕事からコンストラクタの仕事へ
      と変更。メンバ変数(m_pcShareData)の使用準備をコンストラクタで済ませるのは自然。
      これにより派生クラス(CControlProcess/CNormalProcess)からの明示的な初期化要求
      が不要になる。また初期化に失敗した場合には派生クラスの InitializeProcess を呼
      ばないと決めることで、派生クラスでの有効性確認も不要になる。
    • InitializeProcess が 0 を返した場合には MainLoop を呼ばないと決めることで、
      派生クラスの MainLoop 内で行われていた条件判定が不要になる。

    _main/WinMain.cpp

    • MINGW32 対応コードに一本化して #ifdef を除去。コマンドラインを操作するそのコー
      ドは CCommandLine クラスの中へ。
    test_cl.7z について

    パッチ適用後とパッチ適用前のオプション解釈を比較するもの。test_cl.cpp をコンパイルしたものが test_cl.exe。test_cl.exe を sakura.exe に見立てて色々なオプションを引数に起動するのが test_cl.bat。他所の Windows で実行できるかどうか、日本語が表示できるかどうかはやや不安。MSさんに期待。

    CNormalProcess::InitializeProcess の後ろ半分を CNormalProcess::MainLoop へ移動したことのテストにはならない。大まかに3つのコードパスがあり、単純にひとつの範囲を切り貼りするだけで済むものではなかったので、解釈したコマンドラインをどのように実行するかという部分で思わぬ副作用があるかも。

     

    Last edit: ds14050 2018-04-19
    • berryzplus

      berryzplus - 2018-04-19

      自重しないとこういう誰も喜ばない自己満足物質が生み出されます。

      自重してください。お願いします(^^;

      非公開パッチはぼくもたくさん書いてきました。
      プロセス起動部分だけでも何パターンかあります。
      やってみないと分からないこともあるので、試行錯誤は大事だと思います。
      それを公開するかどうかは、試行とは別な話ですよ。

      アグレッシブな修正をしたい人のために、
      ここのプロジェクトに欠けているものが、
      単体テスト一式だと思います。

      これがないと、開発者が何を考えたか、普通の人には分かりません。
      投稿されたパッチを、コミットしてよいものか判断が付きません。
      単体テスト一式があれば少なくとも、何を変えようとしたか(=変わったか)と、
      意図せず変わってしまったものがないかについて確信を得られます。
      問題なさそうならコミットしてもいいんじゃね?という心理トラップにもなります。

      派生プロジェクトではこれをやろうとしてます。(gmock + VCUnitTest)
      ここのプロジェクトでも、使用するテストライブラリを決めて
      何人かで作業すれば1年もかからずに走り出せる状態になると思います。

      超めんどくさい作業ですが、
      もしやる気がある人がいたら協力はしたいと思ってます。

      変更についてツッコミ。
      たくさん書くとごちゃっとするので2つだけ。

      共有データ構造体の初期化を InitializeProcess の仕事からコンストラクタの仕事へ
      と変更。

      失敗できる処理をコンストラクタに任せないでください。
      そんな感じにしたい場合、ファクトリメソッドを用意するのが定石っぽいです。
      ファクトリメソッドで失敗を検出したら例外で抜けるのも定石っぽいです。

      MINGW32 対応コードに一本化して #ifdef を除去。コマンドラインを操作するそのコー
      ドは CCommandLine クラスの中へ。

      windows標準はwWinMainです。
      非標準側にまとめないでください。(vcビルドで問題になるんで。)
      MinGWはwWinMainからのエントリに未対応なので、
      UNICODE版をmakeしたいmingwユーザ向けには#ifdef UNICODEして
      WinMainを定義してやるのが定石っぽいです。

      もちろん、いまのWinMainの#ifdefって変じゃね?って意見には激しく同意です。

       
      • ds14050

        ds14050 - 2018-04-19

        面白い話がいろいろ聞けそうですね。物を知らないということに関して自分はなかなかのものだと自負していますので。

        ここのプロジェクトでも、使用するテストライブラリを決めて

        これですね。とりあえず枠組みを用意して、テストを書くだけという状態に持って行く必要があるでしょう。自分は VCUnitTest の名前も知りません。

        失敗できる処理をコンストラクタに任せないでください。

        失敗できるということは、共有メモリが利用できない場合でもサクラエディタは動作を続けられるということでしょうか。印象だけですけど、即死亡級の失敗かなと思っていました。

        失敗を検出したら例外で抜ける

        これこそまさにコンストラクタに許されている唯一の通知手段です。

        windows標準はwWinMainです。

        そうでしょうか。WinMain が使えなくなる日が来ますか? Unicode対応のためお手軽に wWinMain(_tWinMain)を利用するか GetCommandLine を使用するかという違いで、すでに GetCommandLine を利用するコードがあるわけです。その判断は、コードの分裂を避けたい、GetModuleName という環境に直接依存したパラメータを関数の引数という扱いやすいパラメータに移したいという動機に優先しますか? これはテストのしやすさに影響することです。

        たくさん書くとごちゃっとするので2つだけ。

        出し惜しみはやめましょう。こてんぱんにしてやってください。言われなければこの自重しないパッチもそんなに悪くないんじゃねと本気で考えているような、そんな人間ですよこれを書いたのは。

        もっとも、berryzplus さんがこのプロジェクトのために盾になる理由があると考えることはできないのかもしれませんが。

         

        Last edit: ds14050 2018-04-19
        • berryzplus

          berryzplus - 2018-04-20

          自分は VCUnitTest の名前も知りません。

          VCCppUnitTestの誤りでした。
          cppunittestのVC++特化版です。
          Community(professional)で使う場合、カバレッジを取れるメリットがあります。

          これこそまさにコンストラクタに許されている唯一の通知手段です。

          コンストラクタは最小限の初期化のみを行うべきです。
          例外を投げるコンストラクタを持つクラスは継承が難しくなります。

          疑似コードで説明しましょう。
          newするとメモリ不足例外が発生する状況だと思って読んでください。

          class A {
          char* pszA;
          A() : pszA(NULL) {
          pszA = new char[1];
          }
          ~A() { delete pszA;}
          virtual void doSomething() {}
          };

          classB : public A {
          char* pszB;
          B() : pszB(NULL) {
          pszB = new char[1];
          }
          ~B() { delete pszB;}
          virtual void doSomething() {}
          };

          main() {
          try {
          B b;
          b.doSomething();
          }catch(...){}
          }

          class Bのコンストラクタで例外発生した場合は問題なく動作すると思われます。
          pszBにNULLがセットされた後に例外が発生するので、
          delete pszBは問題なく処理され、delete pszAも問題ないはずです。

          class Aのコンストラクタで例外発生した場合はどうでしょう。
          作成しようとしているのはclass Bのインスタンスなので、
          B::Bの処理に先立ってA::Aが呼ばれているはずです。
          pszBに値がセットされる前にA::Aの例外で落ちるかたちになります。
          pszB未定義の状態でdelete pszBが呼ばれるとどうなりますか?

          これがコンストラクタは例外を投げるべきではない、という理由です。

          WinMain が使えなくなる日が来ますか?

          ある意味で、既に使えなくなっています。
          UNICODEビルド時にもWinMainを定義しなければならない事象は
          MinGW/GCC固有の問題でいずれ是正すべきバグという認識です。

          その判断は、コードの分裂を避けたい、GetModuleName という環境に直接依存したパラメータを関数の引数という扱いやすいパラメータに移したいという動機に優先しますか?

          誤解されているようですが、その辺りは問題ないと思っています。

          問題はwWinMainをWinMainにまとめていることで、
          これをやるとVC++ではコンパイルエラーになることなんです。
          ご存じだと思いますがtWinMainはMinGWにも取り込まれています。
          問題事象はあくまでMinGWのCRTがwWinMainをエントリポイントとしてスタートアップできないことなので、対応はMinGW側コードに行うべきだと思います。

          これはテストのしやすさに影響することです。

          文脈上少し関係ないですが、大事なことなので拾います。
          プログラムのコーディングはテストのしやすさを優先にして行うべきです。
          何よりも優先すべきことは正しく動くことで、テストしやすいってことは正しく動いていることを確認しやすいってことだからです。

          Make it right before you make it faster.
          速度最適化をする前に正しく動くようにせよ

          言われなければこの自重しないパッチもそんなに悪くないんじゃね

          それは同意します。
          もう少し小出しにしたほうが良さそうだと感じましたが、
          ダメなパッチじゃないと思います。

          柔軟な変更を許容するプロジェクトだったとしたら
          普通に取り込まれてしかるべきだと思います。

           
          • ds14050

            ds14050 - 2018-04-20

            class B のコンストラクトが完了していないのに delete pszB; が実行される心配をする必要はないでしょう。自分が知らない C++ にありがちな罠があるのでしたら是非教えてください。C++ は本当に気を抜けない言語ですから。

            言語の話ではなくサクラエディタの話をしますと、CProcess のコンストラクタで例外を投げるようなパッチを書いてはいません。初期化失敗の目印として NULLポインタを保存しておき、その場合は Run メソッドで意味のあることをしないことでプロセスを速やかな終了に導いています。

            自分はパッチの開発を VC++ 2008 Express Edition で行っています。コンパイルもリンクもできています。新しい VC++ では WinMain を使う方法がないのでしょうか。「WinMain が使えなくなる日が来ますか?」と書いたのは自分ですが、すでにその日が来ていると?

            ちなみに GetCommandLine を使う方法は WinMain/wWinMain 両対応です。関数の名前と引数リスト部分にだけ #ifdef を残すことになるでしょうか。

             

            Last edit: ds14050 2018-04-21
            • ds14050

              ds14050 - 2018-04-20

              言われなければこの自重しないパッチもそんなに悪くないんじゃね

              それは同意します。
              もう少し小出しにしたほうが良さそうだと感じましたが、
              ダメなパッチじゃないと思います。

              (かすれていて文字が読めない)

              読んでいただけて評価していただけて嬉しいです!

               

              Last edit: ds14050 2018-04-20
            • berryzplus

              berryzplus - 2018-04-21

              class B のコンストラクトが完了していないのに delete pszB; が実行される心配をする必要はないでしょう

              class Bのメンバが1個じゃなく、いくつかのメンバでリソースを確保し、いくつかのメンバが未定義な状態で落ちるケースも考えてみてください。デストラクタが呼ばれないのは正しいですか?デストラクタが呼ばれた場合どうなりますか?
              昔どこかのc++erの人がブログに書いてたのを見て鵜呑みにしてた情報です。環境依存がある、という話でしたが細かい検証はしていません。リスクを上回るメリットがあるならいいんですけど、コンストラクタで例外を投げるメリットに何があるかよく分っていないです。

              その場合は Run メソッドで意味のあることをしないことでプロセスを速やかな終了に導いています。

              InitializeProcessがRunの中で実行されてるのが変な話になる要因なのかなぁ。
              よくある起動時コードはInitializeに失敗したらRunしない、という構造です。
              sakuraのCProcess::Runは内部でInitializeProcessを呼んで、
              問題なければメッセージループに入ります。
              Runを呼ぶ時点で初期化が完了してないっていう不思議な状態です。

              自分はパッチの開発を VC++ 2008 Express Edition で行っています。

              うちはvisual studio 2013 professional です。
              まさかの「おま環」?
              「WinMainはオーバーロードできません」と怒られます。

              引数リスト部分にだけ #ifdef を残すことになるでしょうか。

              趣味嗜好の問題かもしれません。
              引数リストの途中に#ifdefをいれるとIDEが認識してくれないことがあるので
              これはやです(^^;
              やるなら関数全体を#ifdefで括る方向がいいです。

               
              • ds14050

                ds14050 - 2018-04-21

                デストラクタが呼ばれないのは正しいですか?デストラクタが呼ばれた場合どうなりますか?

                『Exceptional C++』をぱらぱらめくってみましたが正しいはずです。クラスの配列を new してコンストラクタのどれかが例外を投げた場合でも「完全に生成された T オブジェクトが適切に削除される」と 30ページに書かれています。

                弱点としては完全に生成された T オブジェクトのみが削除されるとは書かれていないことでしょうか。あとはコンパイラの実装に固有の問題があるとしても知識がありません。

                パッチの暫定最新版を投稿しないのが悪いのですが「関数の名前と引数リスト部分」とはこういうことです。

                #ifdef __MINGW32__
                int WINAPI WinMain(
                    HINSTANCE hInstance,        //!< handle to current instance
                    HINSTANCE hPrevInstance,    //!< handle to previous instance
                    LPSTR     lpCmdLineA,       //!< pointer to command line
                    int       nCmdShow      //!< show state of window
                )
                #else
                int WINAPI _tWinMain(
                    HINSTANCE hInstance,        //!< handle to current instance
                    HINSTANCE hPrevInstance,    //!< handle to previous instance
                    LPTSTR    lpCmdLine,        //!< pointer to command line
                    int       nCmdShow      //!< show state of window
                )
                #endif
                
                 
                • ds14050

                  ds14050 - 2018-04-22

                  『More Exceptional C++』の第3章がもろにオブジェクトの生存期間というものを C++ の基本的概念のひとつに位置づけて理解を深める章になっています。著者を信じるなら、構築されなかったオブジェクトのデストラクタは呼ばれない。「構築されなかった」とは自分がまとめた言葉であり、その意味するところが紙幅を費やして具体的に述べられています。

                  不面目ながら、C++ の規格書というものの存在をこの目で確かめたことはありません。それっぽい本の記述を信じるのみです。

                   
                  • ds14050

                    ds14050 - 2018-04-22

                    よくある起動時コードはInitializeに失敗したらRunしない、という構造です。
                    sakuraのCProcess::Runは内部でInitializeProcessを呼んで、
                    問題なければメッセージループに入ります。
                    Runを呼ぶ時点で初期化が完了してないっていう不思議な状態です。

                    視点を考えてみましょう。CProcess::Run を呼ぶのは WinMain 関数であり Windows プログラムのエントリポイントです。アプリケーションにとって内部と外部の界面であり、どちらかといえば外側です。Run は外に向けた名前。

                    InitializeProcess, MainLoop, OnExitProcess という名前はアプリケーション内部、具体的には CControlProcess と CNormalProcess に向けた名前です。ごっちゃにしてはいけない。

                    付け加えるとそれら3つの名前はなんだっていいんです。それらがどのように構成され呼び出されるかが本質です。その意味で MainLoop と OnExitProcess が分かれていることには必然性がない。パッチにおいて InitializeProcess の後半から切り出した処理を AfterInitializeProcess という新メンバとはせず MainLoop の前半に移したのも、実質的な差異、分ける必然性がなかったからです。

                     

                    Last edit: ds14050 2018-04-22
                  • berryzplus

                    berryzplus - 2018-04-22

                    WinMainの話は理解しました。
                    引数のlpCmdLineAは使わないわけですね。
                    試せていないけど、それでリンクも通るわけですね。
                    だったらそれでもいいと思います。

                    『More Exceptional C++』の第3章がもろにオブジェクトの生存期間

                    書籍で語られているのはまっとうなオブジェクト定義を行ったクラスの話で、それはそれで正しいと思います。
                    オブジェクトメンバの解放コードは、デストラクタに書かなくても動きます。
                    std::wstringをメンバに持つクラスが構築途中でコケた場合、
                    std::wstringの構築が終わっていればstd::wstringはちゃんと破棄されます。
                    std::wstringの構築が終わってなければstd::wstringのデストラクタは呼ばれません。

                    ぼくが言ってるのは、構築中に例外が発生した場合に自分で書いたデストラクタが呼ばれるか、呼ばれるとして適切に動作するか、呼ばれないとして問題はないか、ということです。
                    書かなくても呼び出されるオブジェクトメンバの解放の話はしていません。

                    自分で確保した複数のリソースを順番に解放する、つまり、
                    デストラクタで2つ以上のリソースを解放するクラスは、
                    普通にあると思います。

                    サクラエディタは生のwindows apiを扱います。生のwindows apiで扱うリソースはほとんどがハンドル型で管理されます。ハンドル型はC++的なオブジェクトではないので、ラッパークラスを書かない限り解放コードが必要です。解放しないとリソースリークが発生します。メモリポインタの開放忘れだとメモリリークになりますよね。

                    リソースが1個だけなら、確保失敗→解放しない、でいいと思います。
                    しかし、サクラエディタを構成するクラス群の多くは複数のリソースメンバを持っています。現状はリソースをコンストラクタで確保するようなことはしていないので問題になっていません。もし「失敗したら例外投げればいいからコンストラクタでリソース確保すればいいじゃん」という話にすると、大変めんどくさいことになるような気がしています。

                    リソースを何も確保せず、デストラクタで解放処理を行わないクラスだったら問題は起きないわけですが、「この場合はOK」「この場合はNG」にすると、個別判断がめんどくさいと思うのです。コンストラクタでは失敗する可能性のあるリソース確保を行わない、としておけば一律の対応ができてラクだと思います。

                    逆アプローチで「解放が必要な非オブジェクトリソースを2つ以上持ってはならない」にすることも可能だとは思います。ただこれは既存コードへの影響がでかいので、努力目標にとどめたほうが良さそうです。

                    なんか、「コンストラクタは例外を投げてはいけない」の話が、
                    「複数のリソースを確保するコンストラクタは例外を投げてはいけない」にすり替わった気がします。

                    結論として、切り分けが面倒だから「コンストラクタは例外を投げない」にしようぜ、と言ってるのでブレてないと言えなくもないんだけど、話大きくぶれましたね…。

                     
                    • ds14050

                      ds14050 - 2018-04-22

                      構築中に例外が発生した場合に自分で書いたデストラクタが呼ばれるか、呼ばれるとして適切に動作するか、呼ばれないとして問題はないか、ということです。

                      継承が難しくなるというのが最初の話でしたから、基本クラスの構築中に例外が発生したという想定で構いませんか。その場合そもそも派生クラスのコンストラクタが呼ばれないのだからリソース獲得の機会がなく解放の必要がありません。

                      初期化リストで生ポインタメンバなどリソースを管理しないメンバを、確保したリソースで初期化していた場合は違います。これは自分のデストラクタが呼ばれないのだから解放されない。呼ばれたとしてもそのメンバが有効なリソースを指しているのか未初期化なのか区別がつきません。 しかし自分が書いている範囲で解決できることです。

                      auto_ptr などの管理オブジェクトに引き渡す以外のリソース(HWNDなど)を初期化リストで獲得しなければいいんです。0 で初期化してコンストラクタ内の try ブロックで new するなりすれば良い。これは初期化リストのすべてが例外を投げないだろうと勝手な前提を設けるより、よっぽど健全で堅固な方法です。


                      ひとつ berryzplus さんの主張を補強するケースを思いつきました。auto_ptr を例にとります。auto_ptr は普通こう使います。

                      auto_ptr<int> pInt(new int);
                      

                      new int には成功したけれど auto_ptr が実装の都合上継承していた基本クラスが例外を投げたことにより auto_ptr のコンストラクタまで実行が移らなかったとします。この場合 new int した結果のメモリ領域を解放する機会が失われます。auto_ptr のコンストラクタが例外を投げないことを保証してくれなければ、その利用がたいへん面倒なことになる。そして auto_ptr がその保証をするためには基本クラスにまで遡って保証を徹底しなければならない。

                      auto_ptr を例にとりましたがたとえば CEditWnd は名前からウィンドウを管理するクラスでしょう。そのコンストラクタがウィンドウハンドルを引数に取り、同時にその破棄を含めた管理責任が授受されるとします。そうすると CEditWnd は自身のコンストラクタで発生する例外は制御できるけれど、その基本クラスで発生する例外には対処できない。これは継承の代わりに包含を選んでも同じ事で、そういうメンバは CEditWnd のコンストラクタで動的に確保するしか選択肢がなくなる。これはたしかに面倒です。

                      しかし、パッチの話をしましょう。CProcess のコンストラクタには try ブロックが必要だという主張でよろしいですか? こうです。

                      CProcess::CProcess(
                          HINSTANCE  hInstance,       //!< handle to process instance
                          const CCommandLine& cl
                      )
                      : m_hInstance( hInstance )
                      , m_CommandLine(cl)
                      , m_pcShareData(0)
                      , m_hWnd( 0 )
                      #ifdef USE_CRASHDUMP
                      , m_pfnMiniDumpWriteDump(NULL)
                      #endif
                      { try {
                          CShareData* pSD = CShareData::getInstance();
                          /* 共有データ構造体のアドレスを返す */
                          if (pSD && pSD->InitShareData()) {
                              m_pcShareData = pSD;
                          } else {
                              //  適切なデータを得られなかった
                              ::MYMESSAGEBOX( NULL, MB_OK | MB_ICONERROR,
                                  GSTR_APPNAME, _T("異なるバージョンのエディタを同時に起動することはできません。") );
                          }
                      } catch(...) {
                      } }
                      
                       

                      Last edit: ds14050 2018-04-22
                      • ds14050

                        ds14050 - 2018-04-22

                        パッチの暫定最新版です。WinMain.cpp でも例外について考えました。

                         
                        • berryzplus

                          berryzplus - 2018-04-23

                          具体的な実装について文句はなかったんですが、
                          ↓の書き込みに対して突っ込んでいました。

                          失敗を検出したら例外で抜ける

                          これこそまさにコンストラクタに許されている唯一の通知手段です。

                          月末でやや忙しいので、週末まで時間をください。

                           
                      • berryzplus

                        berryzplus - 2018-04-23

                        サクラエディタのコードの中には、通常は失敗しない処理がコケた時にメッセージを表示するコードがちらほらあると感じています。

                        通常は失敗しない処理がコケた時・・・
                        まさに例外の使いどころだと思いますが、
                        普通にif(...){正常処理}else{メッセージ表示;return;}になってるところが結構あります。

                        たぶん、あるべきは↓な感じにして例外を投げることだと思います。
                        throw std::exception( "異なるバージョンのエディタを同時に起動することはできません。");

                        メッセージ表示は、呼出元でtryして拾ってあげればよいのです。
                        実際には、投げる例外のクラスはどうだとか、多言語対応はどうすんだとか、関係ない例外を間違って拾ったらどうすんだとか、込み入った話題がぐちゃぐちゃありそうに思えますけど。

                        もちろん、例外を投げるようにする場合、引用部分のコードをコンストラクタに移動するのは先に述べていた理由で反対です。個人的には、共有メモリを作成/参照する処理は、プロセス抽象クラスの「初期化」にあたると思います。コントロールプロセスにしてもエディタプロセスにしてもインスタンス化した瞬間からバリバリ働けるような存在ではないので、初期化工程は必要だと思っています。少なくとも「窓」が出来るまでは初期化工程なんではないかな、と。全体像を読み飛ばしてた感があるので、パッチを一通り見直してからもっかいコメントしようと思っています。

                         
1 2 > >> (Page 1 of 2)

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.