After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 599578 - [PATCH] Extended build_filename() utility function
[PATCH] Extended build_filename() utility function
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
2.20.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2009-10-25 19:14 UTC by Fabricio Godoy
Modified: 2011-02-21 11:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Extended build_filename() utility function (17.52 KB, patch)
2009-10-25 19:14 UTC, Fabricio Godoy
none Details | Review
Extended build_filename() utility function (17.51 KB, patch)
2009-10-26 23:46 UTC, Fabricio Godoy
none Details | Review
Extended build_filename() utility function (12.27 KB, patch)
2009-11-29 22:37 UTC, Fabricio Godoy
needs-work Details | Review
Extended build_filename() utility function (11.93 KB, patch)
2010-01-17 01:07 UTC, Fabricio Godoy
none Details | Review

Description Fabricio Godoy 2009-10-25 19:14:27 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 :)
Comment 1 Fabricio Godoy 2009-10-26 23:46:34 UTC
Created attachment 146306 [details] [review]
Extended build_filename() utility function

Done some fixes based on Murray Cumming tips.
Comment 2 Murray Cumming 2009-11-27 13:55:00 UTC
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?
Comment 3 Fabricio Godoy 2009-11-27 14:05:38 UTC
Because improves build_filename to not be limited to std::string. This way let build_filename handle any text variable as Stringify can.
Comment 4 Fabricio Godoy 2009-11-27 14:07:45 UTC
And, of course, let pass up to 9 parameters against 2.
Comment 5 Murray Cumming 2009-11-27 14:32:35 UTC
> 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.
Comment 6 Fabricio Godoy 2009-11-27 15:17:22 UTC
(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.
Comment 7 Murray Cumming 2009-11-27 15:24:31 UTC
> 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();
Comment 8 Fabricio Godoy 2009-11-27 15:54:47 UTC
(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.
Comment 9 Fabricio Godoy 2009-11-29 22:37:14 UTC
Created attachment 148707 [details] [review]
Extended build_filename() utility function

Changed from templated parameters to std::string parameters.
Comment 10 Murray Cumming 2009-11-30 08:14:03 UTC
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.
Comment 11 Fabricio Godoy 2009-11-30 10:31:09 UTC
(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?
Comment 12 Murray Cumming 2009-11-30 10:41:55 UTC
Oh, actually the others are not real unit tests either. Nevermind.
Comment 13 Jonathon Jongsma 2010-01-06 05:42:25 UTC
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
Comment 14 Fabricio Godoy 2010-01-17 01:07:37 UTC
Created attachment 151578 [details] [review]
Extended build_filename() utility function

Comments fixed as stated by Jonathon Jongsma.
Comment 15 Murray Cumming 2011-02-21 11:29:50 UTC
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.