This is an archive of the discontinued LLVM Phabricator instance.

Rename sys::Process::GetArgumentVector -> sys::windows::GetCommandLineArguments
ClosedPublic

Authored by ruiu on Apr 13 2018, 3:16 PM.

Details

Summary

GetArgumentVector (or GetCommandLineArguments) is very Windows-specific.
I think it doesn't make much sense to provide that function from sys::Process.

I also made a change so that the function takes a BumpPtrAllocator
instead of SpecificBumpPtrAllocator. The latter is the class to call
dtors, but since char * is trivially destructible, we should use the
former class.

Event Timeline

ruiu created this revision.Apr 13 2018, 3:16 PM

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.

llvm/lib/Support/Windows/Process.inc
142

AllocateString seems like a better name considering what the function does.

So I think one option here that maybe solves both problems at once is to create two functions in sys::Process.

One called sys::Process::SaveCommandLineArguments and another called sys::Process::GetSavedCommandLineArguments.

In the second one, you assert if the first one has not been called.

Then, call the first one from InitLLVM::InitLLVM constructor. Have the second one not require the argument vector to be passed in, instead save it in a global in sys::Process or something.

This way, anyone using InitLLVM (which should be everyone) has to do nothing, and all command line arguments are available from anywhere in the program, without needing to pass in the args from main. This way it is not Windows specific, and provides value everywhere.

Thoughts?

So I think one option here that maybe solves both problems at once is to create two functions in sys::Process.

Then, call the first one from InitLLVM::InitLLVM constructor. Have the second one not require the argument vector to be passed in, instead save it in a global in sys::Process or something.

This way, anyone using InitLLVM (which should be everyone) has to do nothing, and all command line arguments are available from anywhere in the program, without needing to pass in the args from main. This way it is not Windows specific, and provides value everywhere.

Do we frequently need to get the arguments outside of the main() function? If I recall correctly, most of the tools today have argument processing only in main(), so a function like sys::Process::GetSavedCommandLineArguments wouldn't provide value outside of main(). It's probably also likely that if there were globals in sys::Process for the arguments, people would try to use them directly (assuming they were accessible), rather than calling the function anyway.

@zturner, what scenario do you see where sys::Process::GetSavedCommandLineArguments would be used (other than in main() or InitLLVM)?

ruiu added a comment.Apr 16 2018, 1:20 PM

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.

Does Android really need this functionality? As far as I know, this is actually Windows specific, and the only user of this functionality is InitLLVM at the moment (because I eliminated all the other uses.) The reason why Windows needs this is historical (this is developed in 90s), and it is hard to imagine that some new system does the same mistake again in the future. So I'm not interested in generalizing this to cover non-Windows system. If something is used only by one .cpp file, that feature doesn't (and perhaps shouldn't) live in llvm::sys namespace.

Android doesn't currently need this, but I can see this being needed on other platforms in the future and I think there is value in having a single point where you can guarantee to get normalized command line arguments on any platform (by exposing something like GetArgumentVector).

That said, since it is only used in InitLLVM right now and InitLLVM should be included in any application, I am convinced InitLLVM can replace that single point for getting normalized command line arguments. Do please rename SaveString, but other than that, it looks good.

ruiu updated this revision to Diff 142801.Apr 17 2018, 10:53 AM
ruiu marked an inline comment as done.
  • address a review comment
This revision is now accepted and ready to land.Apr 17 2018, 10:55 AM
This revision was automatically updated to reflect the committed changes.