[PATCH] D45641: Rename sys::Process::GetArgumentVector -> sys::windows::GetCommandLineArguments

[Stella Stamenova via Phabricator via llvm-commits]

stella.stamenova added a comment.

My concern with this change is that once GetArgumentVector no longer exists, any developer who wants to retrieve the arguments as UTF8 on all platforms will need to be aware that there is a windows-specific function needed to do so. By having GetArgumentVector in sys::Process, we have a unified way of retrieving arguments on all platforms and if in the future it was necessary to specialize on say, Android, the interface wouldn't have to change. I dislike the "GetArgumentVector" name because it is rather unclear what it does (and likely the reason why it was not uniformly used), but I do think it should stay in sys::process.



================
Comment at: llvm/lib/Support/Windows/Process.inc:142
 
-static void AllocateAndPush(const SmallVectorImpl<char> &S,
-                            SmallVectorImpl<const char *> &Vector,
-                            SpecificBumpPtrAllocator<char> &Allocator) {
-  char *Buffer = Allocator.Allocate(S.size() + 1);
-  ::memcpy(Buffer, S.data(), S.size());
-  Buffer[S.size()] = '\0';
-  Vector.push_back(Buffer);
+static const char *SaveString(const SmallVectorImpl<char> &S,
+                              BumpPtrAllocator &Alloc) {
----------------
AllocateString seems like a better name considering what the function does.


https://reviews.llvm.org/D45641