gpakosz / whereami

Locate the current running executable and the current running module/library on the file system 🔎
MIT License
472 stars 64 forks source link

Return std::string / C++ implementation #9

Open Flamefire opened 7 years ago

Flamefire commented 7 years ago

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?

gpakosz commented 7 years ago

See the wip-cpp branch.

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

Flamefire commented 7 years ago

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.

gpakosz commented 7 years ago

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 commented 7 years ago

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.

Flamefire commented 7 years ago

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.

gpakosz commented 7 years ago

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.