-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
3d97cfc
to
c95db81
Compare
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
// |
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.
Wrong copyright header.
c95db81
to
5d125da
Compare
#define WszGetBinaryType GetBinaryTypeWrapper | ||
|
||
//Can not use extended syntax | ||
#define WszGetFullPathName GetFullPathNameW |
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.
MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/aa364963%28v=vs.85%29.aspx) says this function can use extended path syntax. Can you please elaborate on your comment?
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.
MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/aa364963%28v=vs.85%29.aspx) says this function can use extended path syntax. Can you please elaborate on your comment?
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.
GetFullPathName can technically take extended syntax, but it doesn't handle it correctly. It roots the path at ?, which means ?\C:..\Foo becomes ?\Foo. In addition it is a little weird as ?\ is supposed to be fully resolved to begin with. Note that GetFullPathName takes > MAX_PATH for normal paths (verified on WinXP, but appears to go back even further).
c03347c
to
dec50f2
Compare
|
||
wcscpy_s(refRetVal->GetBuffer(), len + 1, dummy); | ||
// Get the value Again. | ||
newLen = WszGetEnvironmentVariable(strVar->GetBuffer(), dummy); |
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 for this now. In the original code we had to allocate the buffer first, and then get the environment variable. Now this is all done by WszGetEnvironmentVariable wrapper. Also don't need the surrounding while loop any more.
On the other hand, the if statement in GetEnvironmentVariableWrapper should really be a while loop, since the environment variable can be modified by another thread between two calls to GetEnvironmentVariableW.
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.
changed it
9939847
to
56c68a8
Compare
Private Desktop Testing is also clean |
I'm good with it. |
LGTM. Could you remove [WIP] from the PR title? |
Thanks @ramarag. Was the CoreCLR DDR also clean? |
They are: 1. Wrappers for OS APIs which take or return PATHS 2. Fixing the usage of following Api's: GetEnvironmentVariableW SearchPathW GetShortPathNameW GetLongPathNameW GetModuleFileName Work remaining: Remove fixed size buffers in the VM
56c68a8
to
f98fb85
Compare
The automated coreclr DDR are gone, I did a manual run it was clean |
Initial Support for LongFile in the VM
I will have a series of commits in this PR (which I don't plan to merge until they are complete)
This First commit is to create Wrappers for the WIn32 APIs that take in a path.
The LongFile class provides various functionalities like path normalizations, tell if a path is relative etc.
The APIs which return a path are wrapped is such a way that they take an out parameter of type SString
@JeremyKuhne @JohnChen0 @gkhanna79 PTAL