Skip to content

Conversation

@gorogoro123
Copy link
Contributor

@gorogoro123 gorogoro123 commented Nov 29, 2025

PR対象

  • アプリ(サクラエディタ本体)

カテゴリ

  • 改善

PR の背景

ヘッダファイルに、"using namespace ApiWrap" を記述している。

仕様・動作説明

  1. "using namespace ApiWrap" を削除します。ApiWrap:: を省略せずに記述します。
  2. 変更行の LTEXT("") を L"" に変更します。

PR の影響範囲

影響なし。

テスト内容

変更前後でasmが同じことを確認する。

変更前
; 871  : 				StatusBar_GetRect(hWnd, nIndex, &partRect);

	mov	eax, DWORD PTR [esi+4]
; File C:\work\sakura\sakura_core\apiwrap\CommonControl.h

; 33   : 		return ::SendMessage( hwndStatus, SB_GETRECT, opt, (LPARAM)rect );

	push	1034					; 0000040aH
	push	DWORD PTR [eax]
	call	DWORD PTR __imp__SendMessageW@16


変更後
; 871  : 				ApiWrap::StatusBar_GetRect(hWnd, nIndex, &partRect);

	mov	eax, DWORD PTR [esi+4]
; File C:\work\sakura\sakura_core\apiwrap\CommonControl.h

; 33   : 		return ::SendMessage( hwndStatus, SB_GETRECT, opt, (LPARAM)rect );

	push	1034					; 0000040aH
	push	DWORD PTR [eax]
	call	DWORD PTR __imp__SendMessageW@16

関連 issue, PR

#2064

参考資料

https://google.github.io/styleguide/cppguide.html#Namespaces

@github-actions
Copy link

Test Results

621 tests  ±0   621 ✅ ±0   1m 23s ⏱️ +2s
 78 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3f90001. ± Comparison against base commit 9961b29.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

「修正行数が多すぎて、まともにレビューできない(Pull Requestに問題がある)」の一時判断を付けときます。


::SetDlgItemInt( hwndCancel, IDC_STATIC_HITCOUNT, 0, FALSE );
::DlgItem_SetText( hwndCancel, IDC_STATIC_CURFILE, L" " ); // 2002/09/09 Moca add
ApiWrap::DlgItem_SetText( hwndCancel, IDC_STATIC_CURFILE, L" " ); // 2002/09/09 Moca add
Copy link
Contributor

@berryzplus berryzplus Nov 30, 2025

Choose a reason for hiding this comment

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

指摘じゃないです。

Suggested change
ApiWrap::DlgItem_SetText( hwndCancel, IDC_STATIC_CURFILE, L" " ); // 2002/09/09 Moca add
::SetDlgItemTextW(hwndCancel, IDC_STATIC_CURFILE, L" ");

と書けるように独自関数を定義してやると、読みやすくなるんじゃないかと思ってます。

bool SetDlgItemTextW(HWND hWndDlg, int nDlgItemId, std::wstring_view text) noexcept;

みたいな独自関数を util/window.h あたりに入れてはどうか、という意味ね。

現状の ApiWrap は「なんでAPIをラップしてるか?」が曖昧だと思ってます。
「Windows APIが扱いづらくてラップしたんじゃねぇのかよ」のツッコミ待ちに見える。

if( HIWORD(lResult) == 0 ){ // クライアントエリア内
if( uMsg == WM_LBUTTONDOWN ){
List_SetCurSel( hwnd, LOWORD(lResult) );
ApiWrap::List_SetCurSel( hwnd, LOWORD(lResult) );
Copy link
Contributor

Choose a reason for hiding this comment

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

指摘じゃないです。

Windowsには ListBox と ListView があるので、List_XXX は紛らわしいと思います。
また、 ListBox にコントロールメッセージを送る方法は、 windowsx.h で提供されてます。

マクロですが。

if( 0 < nTextLen ) {
WCHAR* pszText = new WCHAR[nTextLen + 1];
Combo_GetLBText( hwndCtl, iItem, pszText );
ApiWrap::Combo_GetLBText( hwndCtl, iItem, pszText );
Copy link
Contributor

@berryzplus berryzplus Nov 30, 2025

Choose a reason for hiding this comment

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

指摘じゃないです。

Suggested change
ApiWrap::Combo_GetLBText( hwndCtl, iItem, pszText );
::GetCbItemTextW(hwndCtl, iItem, pszText);

みたいに書けるとイメージしやすいのかな。

int GetCbItemTextW(HWND hWndDlg, int nDlgItemId, int iItem, std::span<WCHAR> buffer) noexcept;
int GetCbItemTextW(HWND hWndComboBox, int iItem, std::span<WCHAR> buffer) noexcept;

みたいな独自関数を作る、ということ。

ListBox同様、ComboBoxにもWindows SDKが提供するマクロがあるけど、文字列取得は独自関数を定義したほうがよさげ。

@berryzplus
Copy link
Contributor

berryzplus commented Nov 30, 2025

参考:

ヘッダーファイル内の using namespace ApiWrap; は禁止級のNG行為なので、できれば直したほうがよいです。
もともと using namespace std: してたプロジェクトなので、ある意味仕方ないと思ってます。

「修正量多過ぎ」の対策として採れる方法は2つ。

  • 関数レベルで using namespace ApiWrap; する。
  • cppレベルで using namespace ApiWrap; する。

現状の ApiWrap 関数群は全体的にビミョーなので、「使うのをやめる」が何気に一番合理的かも。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

変更量多いけど、新たな問題を作り込む余地はないと判断して approve します。

対応ありがとうございました。

@berryzplus berryzplus merged commit 38c1872 into sakura-editor:master Dec 5, 2025
13 checks passed
@gorogoro123 gorogoro123 deleted the feature/remove_using_namespace_ApiWrap branch December 6, 2025 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants