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

[WIP] x64での警告修正(CMemory::AllocBuffer の引数を size_t に変更) #524

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Oct 6, 2018

x64での警告修正

CMemory::AllocBuffer の引数を size_t に変更するのを開始点に、その修正によって
増えた警告を減らしながら対応を進める。

@m-tmatma m-tmatma added the x64 x64 対応 label Oct 6, 2018
@m-tmatma
Copy link
Member Author

m-tmatma commented Oct 6, 2018

#522 (comment)
書いていますが、#495, #501, #521, #522 でヘッダのプロトタイプ宣言を変更
しているのは x64 対応の準備です。

@berryzplus
Copy link
Contributor

これ、ACHAR版も修正していく想定ですか?

ACHAR = サクラエディタ独自定義の文字型 typedef char ACHAR
WCHAR = Windowsの文字型 typedef wchar_t WCHAR

初期サクラエディタはcharで作られていて、途中からWCHARが追加されています。
基本的にコピペなんで同じ関数が2つ以上ある状態になってます。

本格的な対応を始める前に ACHAR 版の関数を除去すれば、
対応箇所を最大半分程度に抑えられる可能性があります。

@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 3 times, most recently from 74f7341 to 4d4ab5d Compare October 8, 2018 10:40
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 2 times, most recently from d4b7166 to e70fb7e Compare October 10, 2018 21:43
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 5 times, most recently from 398988b to 5b5da88 Compare October 17, 2018 22:32
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch from 5b5da88 to 36b9176 Compare October 19, 2018 11:19
@m-tmatma
Copy link
Member Author

これ、ACHAR版も修正していく想定ですか?

この PR では構造は変えない予定です。

@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 2 times, most recently from fa5c3b0 to 1ad1b00 Compare October 27, 2018 05:45
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 3 times, most recently from 22eeddc to 27004c4 Compare November 3, 2018 23:00
@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 4, 2018

ほそぼそ作業しているが、作業前のものと比べると警告は増えてしまう。

対応前 : https://ci.appveyor.com/project/sakuraeditor/sakura/builds/20021347
作業中のもの : https://ci.appveyor.com/project/sakuraeditor/sakura/builds/20030372

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 4, 2018

なお、作業前後の警告の CSV を比較するために以下のようなスクリプトを作った。
(CSV ファイルには GitHub の permalink が含まれるので、単純に CSV を比較したときに
変更になっていない部分も差分ができて、なんの警告が増えたか、減ったか調べにくいので
commit hash の部分を xxxx...xxxx に置換するもの)

import os
import re

def process(infile, outfile):
	with open(infile, "r") as fin:
		with open(outfile, "w") as fout:
			for line in fin:
				data = re.sub(r'blob/[a-fA-F0-9]+', r'blob/xxxxxxxxxxxxxxxxxxxxxxxx',line)
				fout.write(data)


for topdir, dirs, files in os.walk('.'):
	for file in files:
		base, ext = os.path.splitext(file)
		
		match = re.search(r'\.compare\.csv$', file)
		if not match and ext == ".csv":
			full_path = os.path.join(topdir, file)
			out_path  = re.sub(r'\.csv$', r'.compare.csv', full_path)
			process(full_path, out_path)

@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 2 times, most recently from b6a6af5 to 6ef7453 Compare November 11, 2018 22:48
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch 2 times, most recently from 71eab93 to 3eced01 Compare December 19, 2018 21:56
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch from 3eced01 to 87685f5 Compare December 25, 2018 22:56
@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch from a95fd1b to a2bb479 Compare May 4, 2020 01:10
@m-tmatma
Copy link
Member Author

m-tmatma commented May 4, 2020

@berryzplus

行き詰ってるならプロジェクトメンバーや他の人を頼ってみてください。
行き詰ってないならそのまま作業を進めてもらって構わんです。

久しぶりに最新に rebase してみたのですが、
CLogicInt が対応の弊害になるのですが、

CLogicInt ってなんのためにあるクラスがご存じでしょうか?

@m-tmatma
Copy link
Member Author

m-tmatma commented May 4, 2020

メモ
https://codom.hatenablog.com/entry/2018/06/19/222336
ssize_t 使えるかと思ったが、
VC では利用できないので SSIZE_T を使うことにした。

@AppVeyorBot
Copy link

Build sakura 1.0.2754 failed (commit b3634cff14 by @m-tmatma)

@AppVeyorBot
Copy link

Build sakura 1.0.2755 completed (commit 108cded7be by @m-tmatma)

@berryzplus
Copy link
Contributor

berryzplus commented May 4, 2020

おお、動いてる 😄

CLogicInt ってなんのためにあるクラスがご存じでしょうか?

ロジック単位とレイアウト単位を取り違えないようにするためのものです。
取り違えるとビルドエラーになるようにできてます。

発覚している問題点がいくつかありますが、NGな変換コードを書けないようにする発想自体は有用だと思っています。

  • 型を分けるためのコード値が [0,1] 想定なので2種類の型しか作れない。
    • サクラエディタ内で取り違えたらマズい数値型は、少なくとも4種類ある。
    • ロジック位置、レイアウト桁、ロジック行、レイアウト行の4つ。
  • メタクラス定義が重いと考えたのか、デバッグでしか有効にならない。
    • 通常のint(?)を引数にとる関数とCStrictIntegerな引数をとる関数とでコードが二重化する。
    • 速度考慮したい処理内では使われない傾向になって、取り違え対策にならない。
    • デバッグ専用だからチェックが甘かったのか、グローバルな比較演算子が誤って実装されています。(ウソでした。訂正します。詳細は [WIP] CStrictIntegerと整数を比較するグローバル演算子の実装を修正する #1268 参照)
  • メタクラスの制約を無効化する Int に気付きにくい。
    • 型制約のおかしい(正:厳しい)CStrictIntegerに対して、型制約のゆるい型CLaxIntegerがあります。
    • 型制約のゆるい型CLaxIntegerは Int と定義されています。

誤記訂正・・・本音でちゃったのかも 😕

@berryzplus
Copy link
Contributor

言ってることブレてしまって申し訳ないのですが、ptrdiff_t はダメなんじゃないか?ってのは撤回しときたいです。

@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch from a2bb479 to 1e36e08 Compare May 4, 2020 23:56
@AppVeyorBot
Copy link

Build sakura 1.0.2763 completed (commit 4bdf6ae726 by @m-tmatma)

@AppVeyorBot
Copy link

Build sakura 1.0.2793 completed (commit 13723f2763 by @m-tmatma)

@m-tmatma m-tmatma force-pushed the feature/x64-CMemory-AllocBuffer-size_t branch from c7259cb to d8cfc6b Compare May 16, 2020 09:10
@AppVeyorBot
Copy link

Build sakura 1.0.2827 completed (commit b5d7bb8af6 by @m-tmatma)

@m-tmatma m-tmatma marked this pull request as draft August 19, 2020 23:23
@berryzplus
Copy link
Contributor

このPRは still work in progress と認識しとります。

@berryzplus
Copy link
Contributor

#1618 で CMemory 変更により中核の変更が意味をなさなくなったと判断したので閉じてしまいます。

@berryzplus berryzplus closed this Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x64 x64 対応
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants