takuto.ikuta added a comment.
Thank you!
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hans added a comment.
In https://reviews.llvm.org/D47578#1127749, @takuto.ikuta wrote:
> Rui-san, can I ask you to merge this?
Committed here:
http://llvm.org/viewvc/llvm-project?view=revision=334602
https://reviews.llvm.org/D47578
___
takuto.ikuta marked 5 inline comments as done.
takuto.ikuta added a comment.
Rui-san, can I ask you to merge this?
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
takuto.ikuta updated this revision to Diff 150685.
takuto.ikuta added a comment.
Follow llvm style
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
llvm/unittests/Support/CommandLineTest.cpp
Index: llvm/unittests/Support/CommandLineTest.cpp
ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.
Well, I don't think that pointing out that we shouldn't convert strings between
UTF8 and UTF16 back and forth is nitpicking, but yeah, this has already been
addressed, so feel free to submit.
thakis added a comment.
ruiu: This review has now gone on for a week, with one cycle per day due to
timezones. Since the comments are fairly minor nits, do you think you could
address them yourself while landing this, in the interest of not spending
another week on this?
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:216
+ wchar_t ModuleName[MAX_PATH];
+ int Length = ::GetModuleFileNameW(NULL, ModuleName, MAX_PATH);
+ if (Length == 0 || Length == MAX_PATH) {
Can't this be size_t?
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:227
return mapWindowsError(GetLastError());
- if (Length > LongPath.capacity()) {
+ if (static_cast(Length) > MAX_PATH) {
// We're not going to try to deal with paths longer than MAX_PATH,
takuto.ikuta marked 2 inline comments as done.
takuto.ikuta added a comment.
Can the patch be merged this time?
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
takuto.ikuta updated this revision to Diff 150515.
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
llvm/unittests/Support/CommandLineTest.cpp
Index: llvm/unittests/Support/CommandLineTest.cpp
===
---
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:240
+ Filename.assign(Base.begin(), Base.end());
+ return ec;
}
The intention of the code is to return a success, so it is less confusing if
you directly return a success (i.e.
takuto.ikuta added a comment.
I see. Changed not to convert many times.
I confirmed that this passed check-lld, check-llvm and check-clang. If this
looks good for you, can I ask you to merge this?
Thanks.
https://reviews.llvm.org/D47578
___
takuto.ikuta updated this revision to Diff 150300.
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
llvm/unittests/Support/CommandLineTest.cpp
Index: llvm/unittests/Support/CommandLineTest.cpp
===
---
ruiu added a comment.
So the best practice is, when you get a UTF-16 string from an Windows API, you
should convert it to UTF-8 as soon as you can so that you can always process
strings as UTF-8. Likewise, you should always keep all string in UTF-8 in
memory and convert them to UTF-16 just
ruiu added a comment.
It seems you converted the same string back and forth between UTF-8 and UTF-16
to call Windows functions and LLVM functions. That's not only a waste of time
(which is not a big problem) but complicates the code.
I'd define `std::error GetExecutableName(std::string )`
takuto.ikuta marked 3 inline comments as done.
takuto.ikuta added a comment.
Thank you for review
Comment at: llvm/lib/Support/Windows/Process.inc:251
+ // This may change argv0 like below,
+ // * clang -> clang.exe (just add extension)
+ // * CLANG_~1.EXE -> clang++.exe
takuto.ikuta updated this revision to Diff 149694.
takuto.ikuta added a comment.
Address comments
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
llvm/unittests/Support/CommandLineTest.cpp
Index: llvm/unittests/Support/CommandLineTest.cpp
ruiu added a comment.
It seems like you are trying a bit too hard to keep the original code which is
not always a good idea to keep the clarity of the code.
So, IIUC, you this function:
- returns a full filename of the current executable. That can be written using
GetModuleFileName and
takuto.ikuta marked an inline comment as done.
takuto.ikuta added a comment.
In https://reviews.llvm.org/D47578#1117874, @rnk wrote:
> I think this would be easy to unit test in
> llvm/unittests/Support/CommandLine.cpp. We'd just check that the filename is
> "SupportTests.exe" on Windows and
takuto.ikuta updated this revision to Diff 149394.
takuto.ikuta added a comment.
Add test and split function
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
llvm/unittests/Support/CommandLineTest.cpp
Index: llvm/unittests/Support/CommandLineTest.cpp
ruiu added inline comments.
Comment at: llvm/lib/Support/Windows/Process.inc:226
+
+static std::error_code GetLongArgv0Path(const wchar_t *Argv0,
+SmallVectorImpl ,
It looks like this function is a bit too complicated. I'd
amccarth added a comment.
In https://reviews.llvm.org/D47578#1117874, @rnk wrote:
> The LLDB test suite isn't in very good shape on Windows. It is complicated to
> set up and build, I don't want to block this fix on @takuto.ikuta setting up
> that build environment. This is a Windows-only
rnk added a comment.
I think this would be easy to unit test in
llvm/unittests/Support/CommandLine.cpp. We'd just check that the filename is
"SupportTests.exe" on Windows and the path is relative after calling this, I
guess. Right? Look at how ProgramTest.cpp does this to get a reasonable
takuto.ikuta added a comment.
In https://reviews.llvm.org/D47578#1117540, @ruiu wrote:
> Looks like this patch contains two unrelated changes. Please separate your
> change from the lit config changes.
First patch made on some wrong assumption, fixed and reverted test config
change.
In
takuto.ikuta updated this revision to Diff 149304.
takuto.ikuta edited the summary of this revision.
https://reviews.llvm.org/D47578
Files:
llvm/lib/Support/Windows/Process.inc
Index: llvm/lib/Support/Windows/Process.inc
===
---
amccarth added a comment.
I was under the impression that some tools rely on the fact that arg[0] is
always expanded to an absolute path. Does this work with lldb and its test
suite?
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
ruiu added a comment.
Looks like this patch contains two unrelated changes. Please separate your
change from the lit config changes.
https://reviews.llvm.org/D47578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
takuto.ikuta created this revision.
takuto.ikuta added reviewers: thakis, rnk.
Herald added a subscriber: hiraditya.
Herald added a reviewer: alexshap.
Even if we support no-canonical-prefix on
clang-cl(https://reviews.llvm.org/D47480), argv0 becomes absolute path in
clang-cl and that embeds
28 matches
Mail list logo
