GNOME Bugzilla – Bug 599578
[PATCH] Extended build_filename() utility function
Last modified: 2011-02-21 11:29:50 UTC
Created attachment 146222 [details] [review] Extended build_filename() utility function See patch attached. Patch based on glibmm-2-20-mm-common branch. It's my first patch, please be nice :)
Created attachment 146306 [details] [review] Extended build_filename() utility function Done some fixes based on Murray Cumming tips.
Sorry, I don't remember giving the tips. But anyway, why are these templated functions instead of just functions that take const std::string& paramter?
Because improves build_filename to not be limited to std::string. This way let build_filename handle any text variable as Stringify can.
And, of course, let pass up to 9 parameters against 2.
> This way let build_filename handle any text variable as Stringify can. Why could't you do this with a const std::string& parameter? What is Stringify? > And, of course, let pass up to 9 parameters against 2. You don't need a templated function to override a function with various numbers of parameters.
(In reply to comment #5) > > This way let build_filename handle any text variable as Stringify can. > > Why could't you do this with a const std::string& parameter? And another types? char, char*, std::wstring, Glib::ustring... > > What is Stringify? http://git.gnome.org/cgit/glibmm/tree/glib/glibmm/ustring.h#n1228 http://git.gnome.org/cgit/glibmm/tree/glib/glibmm/ustring.h#n1327 > > > And, of course, let pass up to 9 parameters against 2. > > You don't need a templated function to override a function with various numbers > of parameters. Sorry, this is not templates related.
> Why could't you do this with a const std::string& parameter? And another types? char, char*, std::wstring, Glib::ustring... You can pass const char* (or string literals) to any function that expects a std::string. You shouldn't use ustring for file paths because the text encoding of file paths is unknown (not necessarily UTF-8). I doubt that we handle wstring in any other part of our API. So, please just try it with std::string. Also, I don't see the need to split things like this over two lines: gchar* path; path = somethingorother();
(In reply to comment #7) > > Why could't you do this with a const std::string& parameter? > And another types? char, char*, std::wstring, Glib::ustring... > > You can pass const char* (or string literals) to any function that expects a > std::string. I'll test. >You shouldn't use ustring for file paths because the text encoding > of file paths is unknown (not necessarily UTF-8). And I can use std::string because it not enforce any specific encoding, right? > > I doubt that we handle wstring in any other part of our API. Maybe dealing with Microsoft Windows API. > > So, please just try it with std::string. I'll soon. > > > Also, I don't see the need to split things like this over two lines: > gchar* path; > path = somethingorother(); I'll fix it.
Created attachment 148707 [details] [review] Extended build_filename() utility function Changed from templated parameters to std::string parameters.
Thanks. That's much better, though I still hope we could find a simpler way to do this. That's not a unit test though. It doesn't test any result. It looks like an example.
(In reply to comment #10) > Thanks. That's much better, though I still hope we could find a simpler way to > do this. > > That's not a unit test though. It doesn't test any result. It looks like an > example. There is some guide or example that I should follow?
Oh, actually the others are not real unit tests either. Nevermind.
Review of attachment 148707 [details] [review]: ::: glib/glibmm/miscutils.h @@ +253,3 @@ std::string build_filename(const std::string& elem1, const std::string& elem2); +/** Creates a filename from two elements using the correct separator for filenames. minor documentation issue, all of these functions claim to create a filename from *two* elements when they actually take more than two elements. @@ +285,3 @@ + * @param elem4 The fourth element in the path. + * @param elem5 The fifth element in the path. + * @param elem9 The ninth element in the path. extra param (elem9) in the documentation here
Created attachment 151578 [details] [review] Extended build_filename() utility function Comments fixed as stated by Jonathon Jongsma.
OK. It feels ugly, because of the arbitrary 9 maximum, but I can see how it's useful. Pushed to master, so it will be in glibmm 2.28.0. Sorry for the delay.