whereami icon indicating copy to clipboard operation
whereami copied to clipboard

Return std::string / C++ implementation

Open Flamefire opened this issue 7 years ago • 6 comments

The current implementation works with raw pointers. How about porting this to C++ returning a std::string instead (at least as a wrapper) for easier and less error-prone usage?

Flamefire avatar Jul 03 '17 04:07 Flamefire

See the wip-cpp branch.

I don't believe it makes it easier or less error-prone though.

gpakosz avatar Jul 03 '17 05:07 gpakosz

Well I'd make it a lot simpler: Just std::string getExecutablePath() Everything else is (IMO) beyond the scope of this app. No need for basename etc. Also note that std::string[0] should not be used for writing prior to C++11. Use a vector instead and copy.

Easier because: 1 simple call to getExecutablePath() instead of all the get size, alloc, read, free steps which may result in memory leaks.

Flamefire avatar Jul 03 '17 05:07 Flamefire

Well I'd make it a lot simpler: Just std::string getExecutablePath() Everything else is (IMO) beyond the scope of this app. No need for basename etc.

This is just your opinion.

Also note that std::string[0] should not be used for writing prior to C++11. Use a vector instead and copy.

I don't understand why you're mentioning std::string[0], did you have a look at the wip-cpp branch?

Easier because: 1 simple call to getExecutablePath() instead of all the get size, alloc, read, free steps which may result in memory leaks.

There are many situations where people want to have control of memory allocations and are willing to do so. They won't make mistakes.

gpakosz avatar Jul 03 '17 05:07 gpakosz

I don't understand why you're mentioning std::string[0], did you have a look at the wip-cpp branch?

If you're thinking about the standard not mandating std::string's storage to be contiguous prior to C++11, well I've yet to see an STL implementation that doesn't use contiguous storage for std::string.

gpakosz avatar Jul 03 '17 06:07 gpakosz

This is just your opinion.

Yes, therefore the "IMO". Reasoning would be that there are many libraries that can do path handling already (boost, C++17, ...) so I would not include it in a library whose purpose is getting the executable path. Of course it might be convenient for users not wanting to use an extra library, no question there.

I don't understand why you're mentioning std::string[0], did you have a look at the wip-cpp branch?

Yes: https://github.com/gpakosz/whereami/blob/7896790949413722a59a68350fdb2bdeb55f5e4a/src/whereami%2B%2B.cpp#L94 and yes I also agree that the STL implementations all use contiguous storage. But I heard, that some may have used linked lists, extra copy on access, shared pointers etc. and there is lots of discussion on whether using &std::string[0] is safe (prior C++11) so I wanted to mention it. Just "what-if" someone comes along an old, proprietary library that tried to be very clever?

Ok, explicit memory control is a good reason. But often you don't need that, that's why I was suggesting the C++ interface. Did not see the wip branch, so thanks for pointing that out.

Flamefire avatar Jul 03 '17 09:07 Flamefire

Reasoning would be that there are many libraries that can do path handling already (boost, C++17, ...) so I would not include it in a library whose purpose is getting the executable path.

The C++17 filesystem library is well, available from C++17 only. And AFAIK it comes from Boost which is seen as overly bloated and over-engineered by a non negligible fraction of C++ developers. Given that, it was a little effort to add a way to report the dirname part of the executable or module path.

Just "what-if" someone comes along an old, proprietary library that tried to be very clever?

Well, maybe such an STL exists in the wild. I've never seen one in production so far. Hence the choice.

I started the wip-cpp branch out of curiosity and at that point, based on feedback I collected, I still believe it brings very little value to the table.

gpakosz avatar Jul 03 '17 10:07 gpakosz