Page MenuHomeFreeBSD
Differential D10420

bsdgrep trivial improvement: define REG_NOSPEC to be a no-op if not defined, use REG_NOSPEC instead of magic number
ClosedPublic
Actions

Authored by kevans on Apr 18 2017, 2:50 PM.
Tags
None
Referenced Files
F125521257: D10420.diff
Fri, Aug 8, 4:51 PM
F125498405: D10420.id27574.diff
Fri, Aug 8, 10:22 AM
Unknown Object (File)
Thu, Aug 7, 4:53 PM
Unknown Object (File)
Mon, Jul 21, 10:55 AM
Unknown Object (File)
Mon, Jul 21, 10:53 AM
Unknown Object (File)
Mon, Jul 21, 10:31 AM
Unknown Object (File)
Mon, Jul 21, 10:13 AM
Unknown Object (File)
Sun, Jul 13, 10:26 AM
View All Files
Subscribers
None

Details

Reviewers
emaste
Commits
rS317705: bsdgrep: avoid use of magic number for REG_NOSPEC
Summary

At the moment, glibc's cflags are not up to 0020 in value. Future proof this by using REG_NOSPEC directly and defining it to 0 for a no-op in other implementations where it's not defined.

Clearly we can define REG_NOSPEC differently if we pair bsdgrep with another regex(3) implementation that uses a different name, so this is the safe route.

Test Plan

Ensure no regressions with Kyua test suite

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8778
Build 9134: arc lint + arc unit

Event Timeline

kevans created this revision.Apr 18 2017, 2:50 PM
Harbormaster completed remote builds in B8776: Diff 27517.Apr 18 2017, 2:50 PM
kevans updated this revision to Diff 27518.Apr 18 2017, 2:53 PM
  • Fix line length
Harbormaster completed remote builds in B8777: Diff 27518.Apr 18 2017, 2:53 PM
kevans updated this revision to Diff 27519.Apr 18 2017, 2:54 PM
  • Apologies, typo
Harbormaster completed remote builds in B8778: Diff 27519.Apr 18 2017, 2:54 PM
emaste edited edge metadata.Apr 20 2017, 12:56 AM

I agree this is better than 0020. But maybe we should have GREP_FIXED be disabled if neither REG_NOSPEC or REG_LITERAL are defined?

Do we have positive and negative test cases for grep / fgrep / egrep already?

kevans added a comment.Apr 20 2017, 3:06 AM
In D10420#216375, @emaste wrote:

I agree this is better than 0020. But maybe we should have GREP_FIXED be disabled if neiter REG_NOSPEC or REG_LITERAL are defined?

I considered this, but I've never actually seen an implementation that defined REG_LITERAL -- after reading the commment, I assumed it was a typo since I couldn't find any history of a REG_LITERAL in FreeBSDs regex.h and I hadn't seen an implementation that used it. A quick Google search revealed that I was definitively wrong. =)

Should I use this method of #define REG_LITERAL 0 if not defined and just set cflags |= REG_NOSPEC | REG_LITERAL, or would it be cleaner to have two #ifdef blocks inline in this switch? I personally prefer the former, but if there's a precedent it would be good to follow it.

Do we have positive and negative test cases for grep / fgrep / egrep already?

Heh, well, not really. We don't actually even have a single test that uses fgrep/grep -F. egrep has a primitive test for basic branching functionality/matching escaped characters, but the rest of its inclusion was incidental, having come from the one of the PRs for general matching broken-ness with the -o flag.

We could still use improvement in this area. =)

kevans updated this revision to Diff 27574.Apr 20 2017, 2:25 PM
  • Error out at runtime on GREP_FIXED if literal expressions are not supported in regex(3)
Herald added a reviewer: transport. · View Herald TranscriptApr 20 2017, 2:25 PM
Harbormaster completed remote builds in B8810: Diff 27574.Apr 20 2017, 2:25 PM
kevans updated this revision to Diff 27575.Apr 20 2017, 2:26 PM

Other cruft snuck in despite a recent merge with -HEAD...

Harbormaster completed remote builds in B8811: Diff 27575.Apr 20 2017, 2:26 PM
kevans updated this revision to Diff 27576.Apr 20 2017, 2:27 PM

Grrr, arc's behavior with Git is not friendly. Diffs against origin/master, not master

kevans removed a reviewer: transport.Apr 21 2017, 2:27 PM
emaste added inline comments.May 2 2017, 9:05 PM
usr.bin/grep/grep.c
718

This won't work as DEFINED (upper-case) would be treated as a macro. We want #elif defined(), and I'll make the REG_NOSPEC case use defined() as well, for consistency.

emaste added inline comments.May 2 2017, 9:06 PM
usr.bin/grep/grep.c
721

also we want errx here - there is no errno to report.

Closed by commit rS317705: bsdgrep: avoid use of magic number for REG_NOSPEC (authored by emaste). · Explain WhyMay 2 2017, 9:08 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr.bin/
grep/
13 lines

Diff 27519

View Options

usr.bin/grep/grep.c

Loading...