-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ninja UTF8 conversion: Using native windows API for conversion. #1671
Conversation
Sorry, missed the WIP, feel free to ignore my comments. Please add WIP to the title of the PR though. |
Should the final commit be pushed as a new pull-request, removing all the small formatting commits from the log? |
Not as a new PR. You can force push to your branch which will update the PR. I can also squash and merge via GitHub to get rid of the fix-up commits. |
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome to see this! I'm looking forward to see if this resolves some of the bugs we've hit here on the Visual C++ team. I know there's a few more pieces that will be needed, but this change is a necessary first step for Ninja to work in international and mixed language environments and a great improvement. Thanks!
This comment was marked as abuse.
This comment was marked as abuse.
if (!GetTempPath(sizeof(buf), buf)) | ||
TCHAR buf[MAX_PATH]; | ||
// GetTempPath returns zero if no path is found | ||
if(GetTempPath(MAX_PATH, buf) == 0) | ||
return ""; |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a reminder: Ninja doesn't use exceptions.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty return is the same as before, so it should not change any behavoiur.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
src/test.cc
Outdated
if (!GetTempPath(sizeof(buf), buf)) | ||
TCHAR buf[MAX_PATH]; | ||
// GetTempPath returns zero if no path is found | ||
if(GetTempPath(MAX_PATH, buf) == 0) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
@@ -75,10 +75,12 @@ char* mkdtemp(char* name_template) { | |||
|
|||
string GetSystemTempDir() { | |||
#ifdef _WIN32 | |||
char buf[1024]; |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
@jhasse Are there still blockers for accepting and merging this? |
Ping? |
@MrTrillian Fixed the merging conflict, branch is ready and building again. |
@jhasse Can you provide an update on getting this PR merged? Any remaining steps? Thanks! |
I would like to see #1724 fixed first. |
@jhasse That issue has been open for two months without activity, and it doesn't seem related? This PR has been ongoing for almost 5 months. How do we avoid this being prolonged indefinitely? |
By fixing #1724. |
This comment was marked as abuse.
This comment was marked as abuse.
I don't have time to look at this and I don't want to merge PRs without reviewing them. #1724 is next on my to-do list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another look :)
Would be great if the commits could be cleaned up a little bit before merging.
@@ -17,9 +17,12 @@ endif() | |||
# --- compiler flags | |||
if(MSVC) | |||
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>") | |||
string(APPEND CMAKE_CXX_FLAGS " /W4 /GR- /Zc:__cplusplus") | |||
string(APPEND CMAKE_CXX_FLAGS /W4 /GR- /Zc:__cplusplus -DUNICODE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the quotes?
@@ -30,6 +33,10 @@ else() | |||
endif() | |||
endif() | |||
|
|||
if(WIN32) | |||
string(APPEND CMAKE_CXX_FLAGS " -DNOMINMAX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ... maybe target_compile_definitions instead?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using target_compile_definitions (with PRIVATE) the definition won't be inherited by other applications who use ninja as a library.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public header files shouldn't include windows.h at all.
else() | ||
include(CheckCXXCompilerFlag) | ||
if(WIN32) | ||
string(APPEND CMAKE_CXX_FLAGS " -municode") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would -DUNICODE
work with MinGW, too? Then it could be moved to the if(WIN32)
block below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -municode is needed to have mingw use the wmain entry point instead of the ordinary main.
if(WIN32) | ||
string(APPEND CMAKE_CXX_FLAGS " -municode") | ||
endif() | ||
include(CheckCXXCompilerFlag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave tab.
@@ -357,6 +359,9 @@ def binary(name): | |||
pass | |||
if platform.is_mingw(): | |||
cflags += ['-D_WIN32_WINNT=0x0601', '-D__USE_MINGW_ANSI_STDIO=1'] | |||
cflags += ['-DNOMINMAX'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to the line above.
TimeStampFromFileTime(ffd.ftLastWriteTime))); | ||
} while (FindNextFileA(find_handle, &ffd)); | ||
stamps->emplace(std::move(lowername), | ||
TimeStampFromFileTime(ffd.ftLastWriteTime)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove spaces (or clang-format)
@@ -121,18 +122,20 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) { | |||
CloseHandle(nul); | |||
pipe_ = NULL; | |||
// child_ is already NULL; | |||
buf_ = "CreateProcess failed: The system cannot find the file " | |||
buf_ = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to touch this line
@@ -53,7 +54,7 @@ struct Subprocess { | |||
|
|||
private: | |||
Subprocess(bool use_console); | |||
bool Start(struct SubprocessSet* set, const string& command); | |||
bool Start(struct SubprocessSet* set, const StringPiece command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove const (from declaration)
@@ -21,6 +21,9 @@ | |||
#include <windows.h> | |||
#include <io.h> | |||
#include <share.h> | |||
#include <locale> // For UTF-8 conversion | |||
#include <cassert> // For checking string size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.h is already included.
@@ -21,6 +21,9 @@ | |||
#include <windows.h> | |||
#include <io.h> | |||
#include <share.h> | |||
#include <locale> // For UTF-8 conversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
Glad to hear that! I still believe that the *W functions is the way to go. |
Why? Sounds like a huge waste of time to me, only because MS can't lift the 260 character path limit for *A functions, because ... yeah no idea why. |
I see your point. So basically, users on Windows 10 1903 will not need the change. Is there any need to support older versions? If not, this PR might no longer be valid... |
From my point of view, no. |
This comment has been minimized.
This comment has been minimized.
@jhasse Is this PR still wanted or should I just drop it? |
WIP for issue #1195.
Updated version using windows native API for converting between wide and utf8 for increased speed. Fixed buildissue for mingw and added needed defines to the cmake script.
Metrics for building chromium
`
Ninja Master
metric count avg (us) total (ms)
.ninja parse 4849 1931.3 9364.8
canonicalize str 1889079 0.0 58.5
canonicalize path 1985429 0.0 42.8
lookup node 1932008 0.0 36.7
.ninja_log load 1 1504.0 1.5
.ninja_deps load 1 1314.0 1.3
node stat 7363 285.0 2098.4
depfile load 22 184.6 4.1
StartEdge 823 5840.9 4807.1
FinishCommand 822 3970.5 3263.8
CLParser::Parse 753 618.3 465.6
Ninja UTF8-version
metric count avg (us) total (ms)
.ninja parse 4849 2104.7 10205.6
canonicalize str 1889079 0.0 90.5
canonicalize path 1985429 0.0 60.8
lookup node 1933885 0.1 169.9
.ninja_log load 1 2000.0 2.0
.ninja_log recompact 1 14878.0 14.9
node stat 8748 260.8 2281.1
.ninja_deps load 1 3960.0 4.0
depfile load 22 209.7 4.6
StartEdge 823 5909.6 4863.6
FinishCommand 822 908.7 746.9
CLParser::Parse 753 615.2 463.2
`