Page MenuHomePhabricator

Segregate FreeBSD-specific parts from

Authored by kutuzov.viktor.84 on Jul 17 2014, 3:39 AM.

Diff Detail


Event Timeline

kutuzov.viktor.84 retitled this revision from to Segregate FreeBSD-specific parts from
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: kcc, samsonov.
kutuzov.viktor.84 added a subscriber: Unknown Object (MLST).
samsonov added inline comments.Jul 17 2014, 1:41 PM
30 ↗(On Diff #11564)

Can you generalize this with ParseHex() function?

21 ↗(On Diff #11564)

Shouldn't these vars be deleted from this file?

emaste added a subscriber: emaste.Jul 18 2014, 7:20 AM

ReadHex() rewritten via ParseHex(); extra definitions for cached_proc_self_maps_ and cache_lock_ removed from the Linux-specific source file.

samsonov edited edge metadata.Jul 24 2014, 11:18 AM

I think it's better to first remove ReadHex/ReadDecimal in favor of ParseHex/ParseDecimal in a small separate change, and then proceed with this change.

38 ↗(On Diff #11839)

This is weird you don't have (c >= 'A' && c <= 'F'), although these values are handled in ParseHex().

164 ↗(On Diff #11839)

I believe you can safely replace ReadHex with ParseHex here, and get rid of the former.

169 ↗(On Diff #11839)

... And replace this with ParseDecimal() for consistency.

90 ↗(On Diff #11839)

Fix comment near this #endif

That is, it's fine to submit the small piece related to ReadHex/ReadDecimal w/o review.

kutuzov.viktor.84 edited edge metadata.

Fixed as suggested; an alternative approach to parsing numbers proposed.

LGTM modulo nits below.

90 ↗(On Diff #11877)

This function is implemented only on FreeBSD and Linux, but not on Mac. Put it near the ProcSelfMapsBuff definition above.

29 ↗(On Diff #11877)

please change it back to c - 'a' + 10

31 ↗(On Diff #11877)

and c - 'A' + 10

32 ↗(On Diff #11877)

remove the parens

samsonov accepted this revision.Jul 25 2014, 11:19 AM
samsonov edited edge metadata.
This revision is now accepted and ready to land.Jul 25 2014, 11:19 AM
kutuzov.viktor.84 updated this revision to Diff 12228.

Closed by commit rL214955 (authored by vkutuzov).