pcp
[Top] [All Lists]

Re: [pcp] RFC pcp update: pmParseUnitsStr() function

To: "Frank Ch. Eigler" <fche@xxxxxxxxxx>, pcp developers <pcp@xxxxxxxxxxx>
Subject: Re: [pcp] RFC pcp update: pmParseUnitsStr() function
From: Dave Brolley <brolley@xxxxxxxxxx>
Date: Fri, 12 Dec 2014 14:59:23 -0500
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <20141201154517.GK5088@xxxxxxxxxx>
References: <20141201154517.GK5088@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
On 12/01/2014 10:45 AM, Frank Ch. Eigler wrote:
Hi -

As a part of the fetchgroup prototyping, this preparatory step is
ready for your look.  It consists of a new PMAPI function
pmParseUnitsStr() which apprx. reverses pmUnitsStr(), so that pmUnits
may be specified in textual form.  The patchset includes considerable
QA, and a bit of docs & demo code.  I hope to exploit the new facility
in the fetchgroup stuff, pmwebd, python bindings, and other pcp
clients over time.  There is a bonus man-page-typo fix too.

pcpfans.git fche/units-parse branch:
Sorry to be so long in looking at this. I've had a look now and here are my notes:
  • Good catch in correcting scaleSpace and scaleTime (within pmUnits) to be unsigned in pmlookupdesc(3).

  • Are // style comments accepted by the C compilers on all the platforms we support?

  • For the case of the non-existent divisor, why not just clear 'divisor' and set 'divisor_mult' to 1.0 as described by the comment?

  • For the bitfield overflow checks, could that perhaps be automated, and the hard coded limits removed by assigning to the field in 'out' and then checking that they are still equal?

  • In __pmParseUnitsStrPart(), why continue to loop over the various keywords once it is known that 'dimension' is no longer 'd_none' or that the appropriate 'dimXXX' is not longer zero? The test for dimXXX == 0 could be placed around each loop and each loop could simply break once a match has been found. This would eliminate the need to test for 'dimension == d_none' in the first loop. This test could be added around each of the following two loops, or perhaps this is a justified use of a goto (instead of break) within each loop to a label just before the switch statement which follows.

  • Strange -- I see the new files (pmparseunitsstr.3, qa/670, qa/670.out, qa/src/units-parse.c)  in the git diff, but the files do not appear in my repository, which was cloned from your branch and git doesn't seem to know know anything about them at the HEAD. Any ideas? Without these I can't run your test case.

  • Style nit -- PCP coding style does not use a space between a function/macro name and the left paren in function/macro calls.
  Dave
<Prev in Thread] Current Thread [Next in Thread>