add C wrapper for libmetrics

add C wrapper for libmetrics

TEST=unit tests + build&test crosmetrics plugin for flimflam

[Sam Leffler, 2010-06-16 22:58:24]

Most of this is total guesswork so be gentle :-)

TEST=FEATURES=test emerge-x86-generic metrics

[petkov, 2010-06-16 23:51:56]

Thank you very much for working on this.

Mostly style comments. Generally, I think we should still follow the C++ style
guide -- including naming convention, etc.

I'd look at the .h comments first (I started there).

http://codereview.chromium.org/2832008/diff/1/3
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/1/3#newcode9
c_metrics_library.cc:9: #include "metrics_library.h"
Move these two files to after <cstring> (some of the other metrics sources do
the order incorrectly).

http://codereview.chromium.org/2832008/diff/1/3#newcode21
c_metrics_library.cc:21: delete handle;
guard for NULL -- here and below.

http://codereview.chromium.org/2832008/diff/1/3#newcode30
c_metrics_library.cc:30: const char *name, int sample, int min, int max, int
nbuckets) {
Parameter declaration style -- see the comment in the .h.

http://codereview.chromium.org/2832008/diff/1/3#newcode32
c_metrics_library.cc:32: return (int) handle->SendToUMA(std::string(name),
sample, min, max, nbuckets);
Use static_cast<int>.

http://codereview.chromium.org/2832008/diff/1/3#newcode35
c_metrics_library.cc:35: extern "C" int metrics_library_send_enum_to_uma(struct
metrics_library *c_handle,
80 chars but probably OK with the new naming scheme.

http://codereview.chromium.org/2832008/diff/1/3#newcode41
c_metrics_library.cc:41: extern "C" int metrics_library_send_to_autotest(const
char *name, int value) {
Remove this one, I'd say.

http://codereview.chromium.org/2832008/diff/1/4
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/1/4#newcode7
c_metrics_library.h:7: 
Add a brief comment that this is "a C wrapper for the MetricsLibrary class (see
metrics_library.h)."

http://codereview.chromium.org/2832008/diff/1/4#newcode9
c_metrics_library.h:9: struct metrics_library;
Maybe

typedef struct CMetricsLibraryOpaque* CMetricsLibrary;

And then use "CMetricsLibrary" as the handle type.

http://codereview.chromium.org/2832008/diff/1/4#newcode11
c_metrics_library.h:11: struct metrics_library *metrics_library_new(void);
I think we should still follow the C++ style guide. So, maybe:

CMetricsLibrary CMetricsLibraryNew(void);

Btw, do you need the "void"?

http://codereview.chromium.org/2832008/diff/1/4#newcode12
c_metrics_library.h:12: void metrics_library_delete(struct metrics_library
*handle);
void CMetricsLibraryDelete(CMetricsLibrary handle);

etc...

http://codereview.chromium.org/2832008/diff/1/4#newcode13
c_metrics_library.h:13: void metrics_library_init(struct metrics_library
*handle);
Can you add a one line comment what C++ API each function corresponds to? E.g.,

// Wrapper for MetricsLibrary::Init.

http://codereview.chromium.org/2832008/diff/1/4#newcode14
c_metrics_library.h:14: int metrics_library_send_to_uma(struct metrics_library
*handle,
See this on how to align the parameters:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Basically, either align with the first parameter, or use 4-space indent and a
separate line for each parameter.

http://codereview.chromium.org/2832008/diff/1/4#newcode15
c_metrics_library.h:15: const char *c_name, int sample, int min, int max, int
nbuckets);
Change c_name to name and use a different name in the function body (here and
below).

http://codereview.chromium.org/2832008/diff/1/4#newcode17
c_metrics_library.h:17: const char *c_name, int sample, int max);
const char*
space after the *, not before -- chromium style :-/
here and everywhere needed

http://codereview.chromium.org/2832008/diff/1/4#newcode18
c_metrics_library.h:18: int metrics_library_send_to_autotest(const char *c_name,
int value);
I'd avoid wrapping this method -- nobody is really using it and we may deprecate
it and remove it.

http://codereview.chromium.org/2832008/diff/1/5
File c_metrics_library_test.cc (right):

http://codereview.chromium.org/2832008/diff/1/5#newcode1
c_metrics_library_test.cc:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
Move this into the existing metrics_library_test.cc? Class definitions first,
then tests... This way you'll also need fewer changes to the Makefile.

http://codereview.chromium.org/2832008/diff/1/5#newcode16
c_metrics_library_test.cc:16: lib_ = metrics_library_new();
Move this to SetUp.

http://codereview.chromium.org/2832008/diff/1/5#newcode19
c_metrics_library_test.cc:19: metrics_library_delete(lib_);
Move this to TearDown.

[petkov, 2010-06-16 23:55:31]

http://codereview.chromium.org/2832008/diff/1/3
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/1/3#newcode16
c_metrics_library.cc:16: return (struct metrics_library *)handle;
static_cast

http://codereview.chromium.org/2832008/diff/1/3#newcode20
c_metrics_library.cc:20: MetricsLibrary *handle = (MetricsLibrary *)c_handle;
static_cast

[Sam Leffler, 2010-06-17 14:57:44]

Thanks for the help w/ syntax/style.  PTAL

http://codereview.chromium.org/2832008/diff/1/3
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/1/3#newcode9
c_metrics_library.cc:9: #include "metrics_library.h"
On 2010/06/16 23:51:57, petkov wrote:
> Move these two files to after <cstring> (some of the other metrics sources do
> the order incorrectly).
> 

Done.

http://codereview.chromium.org/2832008/diff/1/3#newcode16
c_metrics_library.cc:16: return (struct metrics_library *)handle;
Well I tried that initially but got this:

c_metrics_library.cc:16: error: invalid static_cast from type 'MetricsLibrary*'
to type 'metrics_library*'

with this:

  return static_cast<struct metrics_library *>(handle);

What is the syntax you want and why is this syntax better than old-style c++ for
this case (I understand why it's useful for derived classes)?  Or perhaps I need
to define my opaque class to encap a MetricsLibrary*?  Right now I'm just
forcing types across c/c++ boundaries.

http://codereview.chromium.org/2832008/diff/1/3#newcode21
c_metrics_library.cc:21: delete handle;
delete handles NULL or has c++ changed (been a while since I've written any
large bits of c++)?  or is this a style thing?

http://codereview.chromium.org/2832008/diff/1/3#newcode30
c_metrics_library.cc:30: const char *name, int sample, int min, int max, int
nbuckets) {
Not sure which comment you're referring too but I've tried to match style to the
method decls in metrics_library.h.

http://codereview.chromium.org/2832008/diff/1/3#newcode41
c_metrics_library.cc:41: extern "C" int metrics_library_send_to_autotest(const
char *name, int value) {
On 2010/06/16 23:51:57, petkov wrote:
> Remove this one, I'd say.
> 

Done.

[petkov, 2010-06-17 16:21:08]

It seems that you haven't addressed the naming convention comments (and most of
the other .h comments)?

http://codereview.chromium.org/2832008/diff/1/3
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/1/3#newcode16
c_metrics_library.cc:16: return (struct metrics_library *)handle;
On 2010/06/17 14:57:44, Sam Leffler wrote:
> Well I tried that initially but got this:
> 
> c_metrics_library.cc:16: error: invalid static_cast from type
'MetricsLibrary*'
> to type 'metrics_library*'
> 
> with this:
> 
>   return static_cast<struct metrics_library *>(handle);
> 
> What is the syntax you want and why is this syntax better than old-style c++
for
> this case (I understand why it's useful for derived classes)?  Or perhaps I
need
> to define my opaque class to encap a MetricsLibrary*?  Right now I'm just
> forcing types across c/c++ boundaries.
> 
> 

You probably need reinterpret_cast<> then. See style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

http://codereview.chromium.org/2832008/diff/1/3#newcode21
c_metrics_library.cc:21: delete handle;
On 2010/06/17 14:57:44, Sam Leffler wrote:
> delete handles NULL or has c++ changed (been a while since I've written any
> large bits of c++)?  or is this a style thing?

Ah, didn't know delete ignored NULLs. It's relevant just for the rest then
(e.g., handle->Init()).

http://codereview.chromium.org/2832008/diff/1/3#newcode30
c_metrics_library.cc:30: const char *name, int sample, int min, int max, int
nbuckets) {
On 2010/06/17 14:57:44, Sam Leffler wrote:
> Not sure which comment you're referring too but I've tried to match style to
the
> method decls in metrics_library.h.

The comment that refers to the style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

It's OK now -- but naming convention, etc. is not in.

[Sam Leffler, 2010-06-17 18:03:18]

one more time please; think I've followed style for naming etc.

[petkov, 2010-06-17 18:25:48]

Much better :-)

A few more style nits. You need to move the * and & to the left, per Chromium
coding style :-/

http://codereview.chromium.org/2832008/diff/13001/14003
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/13001/14003#newcode13
c_metrics_library.h:13: CMetricsLibrary CMetricsLibraryNew(void);
Add a one line comment and a blank line to each prototype referring to the C++
API. E.g.,

// C wrapper for MetricsLibrary::MetricsLibrary.

http://codereview.chromium.org/2832008/diff/13001/14003#newcode17
c_metrics_library.h:17: const char *c_name, int sample,
Per original comment: change "c_name" to "name", and use a different name in the
function body.

http://codereview.chromium.org/2832008/diff/13001/14003#newcode20
c_metrics_library.h:20: const char *c_name, int sample, int max);
"const char* " rather than "const char *", per chromium style.

http://codereview.chromium.org/2832008/diff/16001/17001
File Makefile (right):

http://codereview.chromium.org/2832008/diff/16001/17001#newcode80
Makefile:80: %.o: %.c
Do you need this? I don't see any .c files in the CL?

http://codereview.chromium.org/2832008/diff/16001/17002
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/16001/17002#newcode15
c_metrics_library.cc:15: MetricsLibrary *handle = new MetricsLibrary;
"MetricsLibrary* "

http://codereview.chromium.org/2832008/diff/16001/17002#newcode19
c_metrics_library.cc:19: extern "C" void CMetricsLibraryDelete(CMetricsLibrary
c_handle) {
Per style, use the same name for the parameter in the definition as in the
declaration. So, "handle", rather than "c_handle".

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/2832008/diff/16001/17002#newcode20
c_metrics_library.cc:20: MetricsLibrary *handle =
reinterpret_cast<MetricsLibrary *>(c_handle);
<MetricsLibrary*>

http://codereview.chromium.org/2832008/diff/16001/17002#newcode35
c_metrics_library.cc:35: return false;
return 0

http://codereview.chromium.org/2832008/diff/16001/17002#newcode44
c_metrics_library.cc:44: return false;
return 0

http://codereview.chromium.org/2832008/diff/18001/19002#newcode9
c_metrics_library.cc:9: #include <cstring>
You don't need this include, do you?

http://codereview.chromium.org/2832008/diff/18001/19004
File metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/18001/19004#newcode60
metrics_library.h:60: friend class CMetricsLibraryTest;
Let's keep these sorted? So move up?

http://codereview.chromium.org/2832008/diff/18001/19005
File metrics_library_test.cc (right):

http://codereview.chromium.org/2832008/diff/18001/19005#newcode90
metrics_library_test.cc:90: lib_ = CMetricsLibraryNew();
Per original comment: move to SetUp.

http://codereview.chromium.org/2832008/diff/18001/19005#newcode93
metrics_library_test.cc:93: CMetricsLibraryDelete(lib_);
Per original comment: move to TearDown.

http://codereview.chromium.org/2832008/diff/18001/19005#newcode96
metrics_library_test.cc:96: MetricsLibrary &ml =
*reinterpret_cast<MetricsLibrary *>(lib_);
"MetricsLibrary& "
"MetricsLibrary* "

[Sam Leffler, 2010-06-17 18:55:20]

Think I got 'em all.

http://codereview.chromium.org/2832008/diff/13001/14003
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/13001/14003#newcode13
c_metrics_library.h:13: CMetricsLibrary CMetricsLibraryNew(void);
On 2010/06/17 18:25:49, petkov wrote:
> Add a one line comment and a blank line to each prototype referring to the C++
> API. E.g.,
> 
> // C wrapper for MetricsLibrary::MetricsLibrary.

Done.

http://codereview.chromium.org/2832008/diff/13001/14003#newcode17
c_metrics_library.h:17: const char *c_name, int sample,
On 2010/06/17 18:25:49, petkov wrote:
> Per original comment: change "c_name" to "name", and use a different name in
the
> function body.
> 

Done.

http://codereview.chromium.org/2832008/diff/13001/14003#newcode20
c_metrics_library.h:20: const char *c_name, int sample, int max);
On 2010/06/17 18:25:49, petkov wrote:
> "const char* " rather than "const char *", per chromium style.
> 

Done.

http://codereview.chromium.org/2832008/diff/16001/17001
File Makefile (right):

http://codereview.chromium.org/2832008/diff/16001/17001#newcode80
Makefile:80: %.o: %.c
Gone

http://codereview.chromium.org/2832008/diff/16001/17002
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/16001/17002#newcode15
c_metrics_library.cc:15: MetricsLibrary *handle = new MetricsLibrary;
On 2010/06/17 18:25:49, petkov wrote:
> "MetricsLibrary* "
> 

Done.

http://codereview.chromium.org/2832008/diff/16001/17002#newcode19
c_metrics_library.cc:19: extern "C" void CMetricsLibraryDelete(CMetricsLibrary
c_handle) {
On 2010/06/17 18:25:49, petkov wrote:
> Per style, use the same name for the parameter in the definition as in the
> declaration. So, "handle", rather than "c_handle".
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/2832008/diff/16001/17002#newcode20
c_metrics_library.cc:20: MetricsLibrary *handle =
reinterpret_cast<MetricsLibrary *>(c_handle);
On 2010/06/17 18:25:49, petkov wrote:
> <MetricsLibrary*>

Done.

http://codereview.chromium.org/2832008/diff/16001/17002#newcode35
c_metrics_library.cc:35: return false;
On 2010/06/17 18:25:49, petkov wrote:
> return 0
> 

Done.

http://codereview.chromium.org/2832008/diff/16001/17002#newcode44
c_metrics_library.cc:44: return false;
On 2010/06/17 18:25:49, petkov wrote:
> return 0
> 

Done.

http://codereview.chromium.org/2832008/diff/18001/19005
File metrics_library_test.cc (right):

http://codereview.chromium.org/2832008/diff/18001/19005#newcode90
metrics_library_test.cc:90: lib_ = CMetricsLibraryNew();
On 2010/06/17 18:25:49, petkov wrote:
> Per original comment: move to SetUp.
> 

Done.

http://codereview.chromium.org/2832008/diff/18001/19005#newcode93
metrics_library_test.cc:93: CMetricsLibraryDelete(lib_);
On 2010/06/17 18:25:49, petkov wrote:
> Per original comment: move to TearDown.
> 

Done.

http://codereview.chromium.org/2832008/diff/18001/19005#newcode96
metrics_library_test.cc:96: MetricsLibrary &ml =
*reinterpret_cast<MetricsLibrary *>(lib_);
On 2010/06/17 18:25:49, petkov wrote:
> "MetricsLibrary& "
> "MetricsLibrary* "
> 

Done.

[petkov, 2010-06-17 19:29:19]

Almost there...

http://codereview.chromium.org/2832008/diff/26001/27002
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/26001/27002#newcode9
c_metrics_library.cc:9: #include <cstring>
You don't need this include, do you? Remove.

http://codereview.chromium.org/2832008/diff/26001/27002#newcode12
c_metrics_library.cc:12: #include "c_metrics_library.h"
Sort.

http://codereview.chromium.org/2832008/diff/26001/27003
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/26001/27003#newcode24
c_metrics_library.h:24: const char *name, int sample,
"const char* "

http://codereview.chromium.org/2832008/diff/26001/27003#newcode29
c_metrics_library.h:29: const char *name, int sample, int max);
"const char* "

http://codereview.chromium.org/2832008/diff/26001/27005
File metrics_library_test.cc (right):

http://codereview.chromium.org/2832008/diff/26001/27005#newcode5
metrics_library_test.cc:5: #include "c_metrics_library.h"
Would you mind moving these two includes to after gtest/gtest.h?

http://codereview.chromium.org/2832008/diff/26001/27005#newcode91
metrics_library_test.cc:91: MetricsLibrary &ml =
*reinterpret_cast<MetricsLibrary*>(lib_);
"MetricsLibrary& " (space after & rather than before)

[Sam Leffler, 2010-06-17 20:44:58]

Do I get a prize for most reviews / lines of code?

http://codereview.chromium.org/2832008/diff/26001/27002
File c_metrics_library.cc (right):

http://codereview.chromium.org/2832008/diff/26001/27002#newcode9
c_metrics_library.cc:9: #include <cstring>
On 2010/06/17 19:29:19, petkov wrote:
> You don't need this include, do you? Remove.
> 

Done.

http://codereview.chromium.org/2832008/diff/26001/27002#newcode12
c_metrics_library.cc:12: #include "c_metrics_library.h"
On 2010/06/17 19:29:19, petkov wrote:
> Sort.

Done.

http://codereview.chromium.org/2832008/diff/26001/27003
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/26001/27003#newcode24
c_metrics_library.h:24: const char *name, int sample,
On 2010/06/17 19:29:19, petkov wrote:
> "const char* "
> 

Done.

http://codereview.chromium.org/2832008/diff/26001/27003#newcode29
c_metrics_library.h:29: const char *name, int sample, int max);
On 2010/06/17 19:29:19, petkov wrote:
> "const char* "
> 

Done.

http://codereview.chromium.org/2832008/diff/26001/27005
File metrics_library_test.cc (right):

http://codereview.chromium.org/2832008/diff/26001/27005#newcode5
metrics_library_test.cc:5: #include "c_metrics_library.h"
On 2010/06/17 19:29:19, petkov wrote:
> Would you mind moving these two includes to after gtest/gtest.h?
> 

Done.

http://codereview.chromium.org/2832008/diff/26001/27005#newcode91
metrics_library_test.cc:91: MetricsLibrary &ml =
*reinterpret_cast<MetricsLibrary*>(lib_);
On 2010/06/17 19:29:19, petkov wrote:
> "MetricsLibrary& " (space after & rather than before)
> 

Done.

[petkov, 2010-06-17 21:00:32]

Nope but you may get a prize for writing the most beautiful code ever :-)

LGTM

Thanks again for looking into this.

http://codereview.chromium.org/2832008/diff/31001/32003
File c_metrics_library.h (right):

http://codereview.chromium.org/2832008/diff/31001/32003#newcode14
c_metrics_library.h:14: CMetricsLibrary CMetricsLibraryNew(void);
Do you need the "void" here?