8000 Feat: support msys2 by yumetodo · Pull Request #8 · kazatsuyu/fafnir · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Feat: support msys2 #8

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

yumetodo
Copy link
Contributor
@yumetodo yumetodo commented Mar 4, 2018

msys2のgcc(clangはだめ)でもビルドできたらいいなと思い対応してみた。

Copy link
Contributor Author
@yumetodo yumetodo left a comment

Choose a reason for hiding this comment

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

大体C++標準ではstd::ofstreamのctorにwchar_t系の文字列渡せないのが問題。

std::wcerr << "error: " << path << " doesn't exist." << std::endl;
return 1;
}
std::ifstream target(path, std::ios::binary);
std::ifstream target(path.string(), std::ios::binary);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

文字コード変換をサボって雑にやっている。日本語パスが来た時にどうなるのか怖い

@@ -143,7 +144,7 @@ BOOL WINAPI set_file_information_by_handle(
) {
if (information_class == FileRenameInfo) {
auto& info = *static_cast<PFILE_RENAME_INFO>(file_information);
std::ofstream(info.FileName, std::ios::ate | std::ios::binary);
std::ofstream(fs::path(info.FileName).string(), std::ios::ate | std::ios::binary);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

文字コード変換をサボって雑にやっている。日本語パスが来た時にどうなるのか怖い

@yumetodo
Copy link
Contributor Author
yumetodo commented Mar 4, 2018

現在msys2のpacmanで拾えるgccは7.3.0だけれども、たしかgcc8.0.0からfilesystemがexperimentalではなくなるので、そのときは 78af762, 77e6c8a に改修が必要。

@kazatsuyu
Copy link
Owner

http://en.cppreference.com/w/cpp/io/basic_fstream/basic_fstream
std::experimentall::filesystemを使っている時点でどうなのという話ではあるが、このプロジェクトはC++17以前の環境をターゲットにしておらず、C++17ならstd::filesystem::pathconst std::filesystem::path::value_type*を受け取れるので、日本語パスの扱いが不安なコードはあまり入れたくない。
少なくともこのコードは、MSVCの環境ならCP932になってしまうので、入れるなら日本語パスに関するテストを追加した上で対象の環境を切り分けて欲しい。

@yumetodo
Copy link
Contributor Author
yumetodo commented Mar 5, 2018

正直な話Win32APIないしboostで書き換えたい。filesystem使うには時期尚早。そもそもmingwのwchar_t周りは大体壊れていて地雷原を進むようなものなのでgcc8.0.0が来てもまともに実装されていると思えず、Win32API以外で使いたくない。

@kazatsuyu
Copy link
Owner

規模これ以上は大きくならないだろうと思われるためboostには依存したくない。
ターゲットがWindows以外になり得な 8000 ためWin32 APIに依存するのは構わない。

cmake_minimum_required(VERSION 3.10.0)
enable_language(CXX)
set(CMAKE_CXX_STANDARD 17) # C++17...
set(CMAKE_CXX_STANDARD_REQUIRED ON) #...is required...
Copy link
Owner

Choose a reason for hiding this comment

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

この方法に変えてしまうと非mingw環境で動かないので戻してください

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
Owner

Choose a reason for hiding this comment

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

CMakeの-Gオプションで"Visual Studio 15 2017" を指定した場合に set(CMAKE_CXX_STANDARD 17) だと-std=c++17もしくは/std:c++17オプションが付かない。原因を詳しく調査はしていないが、CMAKE_CXX_FLAGSを使っているのはそのため。回避策が分かるなら変更してもいい

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
0