[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [emms-help] [PATCH] New metadata extraction program
From: |
Petteri Hintsanen |
Subject: |
Re: [emms-help] [PATCH] New metadata extraction program |
Date: |
Mon, 19 Oct 2015 23:18:41 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 |
On 19/10/15 16:30, Rasmus wrote:
--- a/AUTHORS
+++ b/AUTHORS
@@ -27,6 +27,7 @@ Ye Wenbin <address@hidden>
Yoni (Johnathan) Rabkin <address@hidden>
mathias.dahl <mathias.dahl>
Rasmus Pank Roulund <address@hidden>
+Petteri Hintsanen <address@hidden>
Maybe make this a separate commit.
OK.
Would it be better to use old-style loops than making this requirement?
Personally, I think C++2011 is fine by now.
For-loops are not the only C++11 thing, auto keyword and initializer
list syntax are there too. But they can be easily replaced if you want
to stick to C++98. It would be a bit uglier.
The naming is confusing. emms-print-metadata is also using taglib, though
not the property list of the C++ binding. I say we stick to one program.
The implementation language doesn’t matter to the end user and the
functionality is the same.
OK. I think this is up to maintainers to decide. I'm happy with any
decision.
+int
+main (int argc, char* argv[])
Is it on purpose that you write int main in two lines?
The style follows GNU Coding Standards.
http://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting
(Personally I do not fancy the style at all, but it is a matter or
taste. Consistency is much more important.)
+ TagLib::FileRef file (argv[1]);
+ if (file.isNull ()) return 1;
Should an helpful error/message be displayed here?
Yes, the original C-version seems to do that too.
Thanks,
Petteri
[emms-help] [PATCH] Add browsing by record label, Petteri Hintsanen, 2015/10/18
Re: [emms-help] [PATCH] New metadata extraction program, Rasmus, 2015/10/19
- Re: [emms-help] [PATCH] New metadata extraction program,
Petteri Hintsanen <=