kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 07/09/2010 04:03 AM, Lorenzo Marcantonio wrote:
> A little useful feature: even if the default unit can be changed between
> inches and mm, the industry is crazy enough to force us with mixed
> design. For example I routinely use imperial units for track size and
> clearance, but drilling is strictly a metric issue...
>
> So I added a little parser to recognize a suffix specification in the
> unit text boxes... so you can put in things like:
> 1in (1 inch)
> 1" (idem)
> 25th (25 thou)
> 25mi (25 mils, the same)
> 6mm (6 mm, obviously)
>
> The rules are: spaces between the number and the unit are accepted, only
> the first two letters are significant.
>
> As a bonus, it also recognize the period (.) as a decimal point
> substituting it with the correct locale character (there was a wishlist
> for it, IIRC). Most useful for number pad fans :D


Some observations and suggestions:

1) If moving away from int for aUnits, must we go to bool?  Can we at
least go to a new enum rather than a bool?  Why burn the bridge to being
able to support other units in the future?  (There is no cost to
preserving the bridge.)


2) The argument name "metric" does not convey what it should.  If you we
were going to stick with bool, (and I don't think we should), at least
"isMetric" would actually say something about the meaning of the
parameter.   I prefer aUnits.


3) Your single line C comments should really be C++ comments and have
blank lines above them.


+    /* Acquire the 'right' decimal point separator */
+    const struct lconv *lc = localeconv();
+    wxChar decimal_point = lc->decimal_point[0];
+    wxString buf(TextValue.Strip(wxString::both));
+    /* Convert the period in decimal point */
+    buf.Replace(wxT("."), wxString(decimal_point, 1));
+    /* Find the end of the numeric part */
+    unsigned brk_point = 0;
+    while (brk_point < buf.Len()) {
+        wxChar ch = buf[brk_point];
+        if (!((ch >= '0' && ch <='9') || (ch == decimal_point))) {
+            break;
+        }
+        ++brk_point;
+    }
+
+    /* Extract the numeric part */
+    buf.Left(brk_point).ToDouble( &dtmp );
+
+    /* Check the optional unit designator (2 ch significant) */
+    wxString unit(buf.Mid(brk_point).Strip(wxString::leading).Left(2).Lower());
+    if (unit == wxT("in") || unit == wxT("\"")) {
+        metric = false;
+    } else if (unit == wxT("mm")) {
+        metric = true;
+    } else if (unit == wxT("mi") || unit == wxT("th")) { /* Mils or thous */
+        metric = false;
+        dtmp /= 1000;
+    }
+    Value = From_User_Unit( metric, dtmp, Internal_Unit );



4) Your bracket placement is not compatible with established style.

5) There should be no spaces after while, for, or if.

6) There should be a space after most any (.


The concept of the patch, if it works, otherwise seems useful.

If you can correct this, it would save any committer some work and is
more likely to be committed.

Thanks,

Dick