Skip to content
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

言語DLLのMakefileのMYDEFINESの指定方法をシンプルに戻す #1389

Merged
merged 1 commit into from Sep 5, 2020

Conversation

berryzplus
Copy link
Contributor

PR の目的

言語DLLのMakefileのMYDEFINESの指定方法をシンプルに戻して、build-gnu.batを修正しなくて済むようにします。

カテゴリ

  • ビルド関連
    • MinGWビルド

PR の背景

#1386 MinGW: Refactor Makefile for Language DLL
リソースのDebug/Release指定漏れを防止するために、_DEBUGNDEBUG が必ず指定されるようにする仕組みを言語DLLのMakefileに入れました。

   ↓

#1381 テストバッチの辻褄を合わせる

Makefileに対するデバッグ/リリース指定に使うMYDEFINESの定義方法を、言語DLL向けMakefileで変えてしまっていたのを本体側Makefileに適用しないといかんです。

この変更を見落としていましたが、本体側の方が素直で分かりやすいと思いますね。

ではそこは戻す方向で。

Originally posted by @berryzplus in #1381 (comment)

PR のメリット

言語DLLのビルドをbuild-gnu.batに組み込む際にbuild-gnu.batを修正しなくて済むようになります。

PR のデメリット (トレードオフとかあれば)

たぶん、ないです。

仕様・動作説明

MYDEFINES のチェック方法を変更します。

  • 変更前: _DEBUGNDEBUG のいずれかが指定されているかチェック、なければ NDEBUG 指定とみなす。
  • 変更後: -D_DEBUG-DNDEBUG のいずれかが指定されているかチェック、なければ -DNDEBUG 指定とみなす。

テスト内容

MYDEFINESの指定を変えてMinGWの言語DLLをビルドして、生成されたDLLを visual studio で開き、埋め込まれたアイコンリソースに問題ないことを確認しました。

  • mingw32-make -f ../../../sakura_lang_en_US/Makefile MYDEFINES= SAKURA_CORE=. OUTDIR=. -j8
    • MYDEFINES 指定なし、Release版になるのでサクラアイコン(ID=210)がピンク。
  • mingw32-make -f ../../../sakura_lang_en_US/Makefile MYDEFINES=-D_DEBUG SAKURA_CORE=. OUTDIR=. -j8
    • MYDEFINES=-D_DEBUG Debug版になるのでサクラアイコン(ID=210)が青。
  • mingw32-make -f ../../../sakura_lang_en_US/Makefile MYDEFINES=-DNDEBUG SAKURA_CORE=. OUTDIR=. -j8
    • MYDEFINES=-DNDEBUG Release版になるのでサクラアイコン(ID=210)がピンク。

PR の影響範囲

MinGWの言語DLLのビルドに影響します。

関連 issue, PR

#1386 MinGW: Refactor Makefile for Language DLL

参考資料

Comment on lines -64 to 68
ifeq (_DEBUG,$(findstring _DEBUG,$(MYDEFINES)))
else ifeq (NDEBUG,$(findstring NDEBUG,$(MYDEFINES)))
ifeq (-D_DEBUG,$(findstring -D_DEBUG,$(MYDEFINES)))
else ifeq (-DNDEBUG,$(findstring -DNDEBUG,$(MYDEFINES)))
else
MYDEFINES += NDEBUG
MYDEFINES += -DNDEBUG
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NDEBUG ってリソースファイルの中では使っていないような気がするのですが、このブロック必要ですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

使ってませんが、あったほうがより良いと思います。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本体の Makefile にもこの部分はないですが、追加する予定でしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あったほうが良い理由は、MSVC側のReleaseビルドで指定されるフラグだからです。
パターンは4つあって、実質的に使われるのは2つですが NDEBUG を指定しなくてよい理由はない気がします。

-- _DEBUG NDEBUG
Debugビルド 定義あり 定義なし
Releaseビルド 定義なし 定義あり
ありえない1 定義なし 定義なし
ありえない2 定義あり 定義あり

このブロックにより ありえない1 の発生を排除することができます。
ありえない2 の排除は無理だと思うので放置します。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本体の Makefile にはこの部分がなくて、言語 DLL の方にだけあるのは統一が取れていないので、両方から削除するか、両方に追加するかどちらかにした方がいいのでは?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

本体の Makefile にはこの部分がなくて、言語 DLL の方にだけあるのは統一が取れていないので、両方から削除するか、両方に追加するかどちらかにした方がいいのでは?

本体に追加したいです。
それはいつか別PRで対処したいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いまやっときます?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

別PRでいいでしょう。
方針が決まらなければ、このブロックが必要かどうかの判断ができないので聞いただけのことです。

@k-takata k-takata added environment 環境関連全般【ChangeLog除外】 MinGW MinGW labels Sep 5, 2020
@AppVeyorBot
Copy link

Copy link
Member

@k-takata k-takata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。マージしちゃいます。

@berryzplus berryzplus merged commit 2f0a862 into sakura-editor:master Sep 5, 2020
@berryzplus berryzplus deleted the feature/fix_mydefines branch September 5, 2020 10:57
@k-takata
Copy link
Member

k-takata commented Sep 5, 2020

よく考えたら、-D_DEBUG-DNDEBUG も指定されていなければ -DNDEBUG を指定するということは、こういう書き方の方が分かりやすいのではと思ったり。

ifeq (,$(findstring -D_DEBUG,$(MYDEFINES)))
ifeq (,$(findstring -DNDEBUG,$(MYDEFINES)))
MYDEFINES += -DNDEBUG
endif
endif

@berryzplus
Copy link
Contributor Author

よく考えたら、-D_DEBUG-DNDEBUG も指定されていなければ -DNDEBUG を指定するということは、こういう書き方の方が分かりやすいのではと思ったり。

ifeq (,$(findstring -D_DEBUG,$(MYDEFINES)))
ifeq (,$(findstring -DNDEBUG,$(MYDEFINES)))
MYDEFINES += -DNDEBUG
endif
endif

んではそれで修正しときます。別PRになるので本体側も合わせて変更したいと思います。

@berryzplus
Copy link
Contributor Author

別PR作って、テストのところで再チェックしてて、MYDEFINES=のときに-DNDEBUGが指定されてなかったことに気付くなど・・・。合わせて対処しようと思います。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
environment 環境関連全般【ChangeLog除外】 MinGW MinGW
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants