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

UTF32BEの文字コード変換クラスが7bit ASCIIを取り込めない問題に対処する #1627

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

タイトル通りです。少なくとも 7bit ASCII を取り込めるようにします。

カテゴリ

  • 不具合修正

PR の背景

#1614 (comment) で書いた通り、UTF32BEの文字コード変換クラスに不具合が見つかっています。

PR のメリット

  • UTF32BEの文字コード変換クラスで 7bit ASCII を取り込めるようになります。

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

  • UTF32BEの文字コード変換クラスが 7bit ASCII 以外の文字を取り込めることを保証するPRではありません。
    修正コード周辺には不審点がいくつかあるので、直しても結局動かないかも知れません。
  • 作成したテストが誤っている可能性がちょっとだけあります。

仕様・動作説明

PR の影響範囲

テスト内容

  1. UTF32LEのテストデータ(リトルエンディアン)をエンディアン変換しただけのテストを追加します。
    コード修正を行う前は、このテストに失敗します。
  2. 想定する修正を適用します。
    コード修正を行った後は、このテストに成功します。

関連 issue, PR

参考資料

@berryzplus
Copy link
Contributor Author

テストを追加しただけだとエラーになりました。
https://github.com/sakura-editor/sakura/pull/1627/checks?check_run_id=2343597585

@sonarcloud
Copy link

sonarcloud bot commented Apr 14, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@berryzplus berryzplus marked this pull request as ready for review April 14, 2021 14:56
@AppVeyorBot
Copy link

Copy link
Member

@kengoide kengoide left a comment

Choose a reason for hiding this comment

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

問題ないものと思います。

@@ -451,6 +451,54 @@ TEST(CCodeBase, codeUtf32Le)

//! googletestの出力に文字セットIDを出力させる
std::ostream& operator << (std::ostream& os, const ECodeType& eCodeType);
/*!
Copy link
Member

Choose a reason for hiding this comment

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

空行の入れ忘れでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

・・・どちらかというと、テストを挿入する場所を間違ってますね。

普通に考えて、452行目の手前に入れるのが適切な気がします。

このPRはこのまま進めて、なんかのついでに直してしまおうと思います。

@berryzplus
Copy link
Contributor Author

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

挿入箇所を間違えてる件については、次回このテストファイルを触るときに対応したいと思います。

@berryzplus berryzplus merged commit ab01e5e into sakura-editor:master Apr 16, 2021
@berryzplus berryzplus deleted the feature/workaround_for_utf32be branch April 16, 2021 00:11
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Oct 4, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants