Replace custom path handling in MRT #908
Conversation
ghost
added
the
dev/MRTCore/mrt/Core/src/MRM.cpp
Outdated
| DWORD lastError = GetLastError(); | ||
| std::unique_ptr<wchar_t[]> pathAllocated; | ||
| wil::unique_cotaskmem_string path; | ||
| RETURN_IF_FAILED((wil::GetModuleFileNameW<wil::unique_cotaskmem_string, 256>(nullptr, path))); |
There was a problem hiding this comment.
You can leave off the , 256 parameter and then remove the additional ( ) on this.
There was a problem hiding this comment.
256 is what i want to override. the default is 128. Many times, the path created by the reunion template is fairly long. Thus i want to bump up the default.
| } | ||
| pointerToPath++; | ||
| } | ||
| PCWSTR name = wil::find_last_path_segment(path.get()); |
There was a problem hiding this comment.
Should this use the std::filesystem APIs instead?
There was a problem hiding this comment.
The file contains no-throwing code only. with std, i am going to worry about whether it throws, and std::filesystem API does throw.
dev/MRTCore/mrt/Core/src/MRM.cpp
Outdated
| DWORD lastError = GetLastError(); | ||
| std::unique_ptr<wchar_t[]> pathAllocated; | ||
| wil::unique_cotaskmem_string path; | ||
| RETURN_IF_FAILED((wil::GetModuleFileNameW<wil::unique_cotaskmem_string, 256>(nullptr, path))); |
There was a problem hiding this comment.
Isn't it better to use MAX_PATH instead of 256 here? MAX_PATH may be greater than 256 after a user modification, no?
There was a problem hiding this comment.
256 is just twice of the default (128). Some people may just don't like MAX_PATH. I doubt anyone is going to modify MAX_PATH. If it does, that's just one more reason to not use MAX_PATH.
There was a problem hiding this comment.
You're trading codebase readability for a perceived micro-optimization. Other folks coming across this code are going to scratch their heads as to why the initial buffer size was not used here. Something to consider.
There was a problem hiding this comment.
It's better to use no number here. WIL always produces a heap-allocated string from this method, and the buffer parameter is just a seed to know where to start growing the buffer. Just don't specify, let WIL do its thing, then there's no Mysterious Constant hanging out here. (Mysterious Constants in library implementations are fine.)
| pointerToPath++; | ||
| } | ||
| PCWSTR name = wil::find_last_path_segment(path.get()); | ||
| wchar_t moduleFilename[MAX_PATH]; |
There was a problem hiding this comment.
883-884 can go into the "// Prep for the second iteration" if block where if name is empty/null, we just break. We can avoid the unnecessary setting of module name to path and then a second iteration.
There was a problem hiding this comment.
at that time, path is already modified, and lost the name.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* Replace custom path handling * use default size
also removed custom build configurations in mrt build task.