Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nominal no_std + alloc support to regex-syntax #477

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ci/script.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ ci/run-shootout-test
cargo test --verbose --manifest-path regex-syntax/Cargo.toml
cargo doc --verbose --manifest-path regex-syntax/Cargo.toml

if [ "$TRAVIS_RUST_VERSION" = "nightly" ]; then
cargo test --verbose --manifest-path regex-syntax/Cargo.toml --no-default-features --features "alloc"
fi

# Run tests on regex-capi crate.
cargo build --verbose --manifest-path regex-capi/Cargo.toml
(cd regex-capi/ctest && ./compile && LD_LIBRARY_PATH=../../target/debug ./test)
Expand Down
7 changes: 7 additions & 0 deletions regex-syntax/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,12 @@ homepage = "https://github.com/rust-lang/regex"
description = "A regular expression parser."
workspace = ".."

[features]
default = ["std"]
# Disable this on-by-default feature and add "alloc" to allow use in no_std builds
std = []
# Required for use in no_std builds, presently nightly-only
alloc = []
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need an alloc feature flag if it will always be required — i.e. unless there are plans for core-only support in the future you could just drop this feature flag and do #![cfg_attr(not(feature = "std"), feature(alloc))].

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to leave the door open for that possibility. We can simplify the cfg statements if those plans don't materialize.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trick we do with Rand: make std depend on alloc. This has the advantage that you can use just #[cfg(feature = "alloc")] to feature-gate modules/items requiring an allocator (though also the disadvantage that sometimes you need to check both: #![cfg_attr(all(feature="alloc", not(feature="std")), feature(alloc))]).


[dependencies]
ucd-util = "0.1.0"
5 changes: 4 additions & 1 deletion regex-syntax/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ Defines an abstract syntax for regular expressions.
*/

use std::cmp::Ordering;
#[cfg(feature = "std")]
use std::error;
use std::fmt;
use prelude::*;

pub use ast::visitor::{Visitor, visit};

Expand Down Expand Up @@ -179,6 +181,7 @@ pub enum ErrorKind {
__Nonexhaustive,
}

#[cfg(feature = "std")]
impl error::Error for Error {
fn description(&self) -> &str {
use self::ErrorKind::*;
Expand Down Expand Up @@ -1488,7 +1491,7 @@ mod tests {
#[test]
#[cfg(any(unix, windows))]
fn no_stack_overflow_on_drop() {
use std::thread;
use std_test::thread;

let run = || {
let span = || Span::splat(Position::new(0, 0, 0));
Expand Down
3 changes: 2 additions & 1 deletion regex-syntax/src/ast/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
This module provides a regular expression parser.
*/

use std::borrow::Borrow;
use prelude::{Borrow, Box, String, ToString, Vec};
use std::cell::{Cell, RefCell};
use std::mem;
use std::result;
Expand Down Expand Up @@ -2255,6 +2255,7 @@ impl<'p, 's, P: Borrow<Parser>> ast::Visitor for NestLimiter<'p, 's, P> {

#[cfg(test)]
mod tests {
use std_test::prelude::v1::*;
use std::ops::Range;

use ast::{self, Ast, Position, Span};
Expand Down
1 change: 1 addition & 0 deletions regex-syntax/src/ast/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ impl<'p, W: fmt::Write> Writer<'p, W> {
#[cfg(test)]
mod tests {
use ast::parse::ParserBuilder;
use std_test::prelude::v1::*;
use super::Printer;

fn roundtrip(given: &str) {
Expand Down
1 change: 1 addition & 0 deletions regex-syntax/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// except according to those terms.

use std::fmt;
use prelude::Vec;

use ast::{self, Ast};

Expand Down
4 changes: 4 additions & 0 deletions regex-syntax/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use std::cmp;
#[cfg(feature = "std")]
use std::error;
use std::fmt;
use std::result;
use prelude::*;

use ast;
use hir;
Expand Down Expand Up @@ -39,6 +41,7 @@ impl From<hir::Error> for Error {
}
}

#[cfg(feature = "std")]
impl error::Error for Error {
fn description(&self) -> &str {
match *self {
Expand Down Expand Up @@ -285,6 +288,7 @@ fn repeat_char(c: char, count: usize) -> String {

#[cfg(test)]
mod tests {
use std_test::prelude::v1::*;
use ast::parse::Parser;

// See: https://github.com/rust-lang/regex/issues/464
Expand Down
1 change: 1 addition & 0 deletions regex-syntax/src/hir/interval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::cmp;
use std::fmt::Debug;
use std::slice;
use std::u8;
use prelude::Vec;

// This module contains an *internal* implementation of interval sets.
//
Expand Down
2 changes: 2 additions & 0 deletions regex-syntax/src/hir/literal/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::fmt;
use std::iter;
use std::mem;
use std::ops;
use prelude::*;

use hir::{self, Hir, HirKind};

Expand Down Expand Up @@ -999,6 +1000,7 @@ fn cls_byte_count(cls: &hir::ClassBytes) -> usize {

#[cfg(test)]
mod tests {
use std_test::prelude::v1::*;
use std::fmt;

use ParserBuilder;
Expand Down
9 changes: 6 additions & 3 deletions regex-syntax/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
/*!
Defines a high-level intermediate representation for regular expressions.
*/

use std::char;
use std::cmp;
#[cfg(feature = "std")]
use std::error;
use std::fmt;
use std::u8;
use prelude::*;

use ast::Span;
use hir::interval::{Interval, IntervalSet, IntervalSetIter};
Expand Down Expand Up @@ -104,6 +107,7 @@ impl ErrorKind {
}
}

#[cfg(feature = "std")]
impl error::Error for Error {
fn description(&self) -> &str {
self.kind.description()
Expand Down Expand Up @@ -209,8 +213,7 @@ impl Hir {
/// Consumes ownership of this HIR expression and returns its underlying
/// `HirKind`.
pub fn into_kind(mut self) -> HirKind {
use std::mem;
mem::replace(&mut self.kind, HirKind::Empty)
::std::mem::replace(&mut self.kind, HirKind::Empty)
}

/// Returns an empty HIR expression.
Expand Down Expand Up @@ -2016,7 +2019,7 @@ mod tests {
#[test]
#[cfg(any(unix, windows))]
fn no_stack_overflow_on_drop() {
use std::thread;
use std_test::thread;

let run = || {
let mut expr = Hir::empty();
Expand Down
1 change: 1 addition & 0 deletions regex-syntax/src/hir/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ impl<'p, W: fmt::Write> Writer<'p, W> {
#[cfg(test)]
mod tests {
use ParserBuilder;
use std_test::prelude::v1::*;
use super::Printer;

fn roundtrip(given: &str, expected: &str) {
Expand Down
3 changes: 2 additions & 1 deletion regex-syntax/src/hir/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Defines a translator that converts an `Ast` to an `Hir`.

use std::cell::{Cell, RefCell};
use std::result;

use prelude::{Box, ToString, Vec};
use ast::{self, Ast, Span, Visitor};
use hir::{self, Error, ErrorKind, Hir};
use unicode::{self, ClassQuery};
Expand Down Expand Up @@ -1094,6 +1094,7 @@ mod tests {
use ast::{self, Ast, Position, Span};
use ast::parse::ParserBuilder;
use hir::{self, Hir, HirKind};
use std_test::prelude::v1::*;
use unicode::{self, ClassQuery};

use super::{TranslatorBuilder, ascii_class};
Expand Down
1 change: 1 addition & 0 deletions regex-syntax/src/hir/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use prelude::Vec;
use hir::{self, Hir, HirKind};

/// A trait for visiting the high-level IR (HIR) in depth first order.
Expand Down
19 changes: 19 additions & 0 deletions regex-syntax/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,21 @@ done automatically in the `regex` crate.

#![deny(missing_docs)]

#![cfg_attr(not(feature = "std"), no_std)]
#![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(alloc))]
#![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(slice_concat_ext))]

#[cfg(test)]
extern crate std as std_test;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this? If std is required for testing, you can just use regular includes from std. If you want to test where std is not available this will fail. It's possible to run many tests without std anyway, though might take some more work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that we want to be able to run our tests against the library compiled in either mode as a way of confirming that there is no accidental behavioral differences.

E.G. cargo test --no-default-features --features "alloc"

IIUIC, because we (now) do extern crate core as std in the main library when in no_std mode (to reduce code diffs), we need to import std under some other name for use in the tests so as not to conflict with the core-renamed-as-std crate being used by the library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even know that you do need std for most of the testing. Have a look at the Rand lib code; we allow many tests to run with cargo test --no-default-features without re-importing std. But it's possible there are other parts of std you do require.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that we could make a decent chunk of the tests run without access to the standard library.

That said, the main goal is testing the library code. AFAICT that library code can be compiled in no_std mode and exercised regardless of whether the test code itself has access to standard library. As as we're getting the library code exercised already, I'm not clear on what value add there would be to go to the effort of triaging and modifying the test code to be able to run without the standard lib.


#[cfg(not(feature = "std"))]
#[macro_use]
extern crate core as std;

#[cfg(all(feature = "alloc", not(feature = "std")))]
#[macro_use]
extern crate alloc;

extern crate ucd_util;

pub use error::{Error, Result};
Expand All @@ -115,9 +130,12 @@ mod either;
mod error;
pub mod hir;
mod parser;
mod prelude;
mod unicode;
mod unicode_tables;

use prelude::String;

/// Escapes all regular expression meta characters in `text`.
///
/// The string returned may be safely used as a literal in a regular
Expand Down Expand Up @@ -199,6 +217,7 @@ pub fn is_word_byte(c: u8) -> bool {

#[cfg(test)]
mod tests {
use std_test::prelude::v1::*;
use super::*;

#[test]
Expand Down
54 changes: 54 additions & 0 deletions regex-syntax/src/prelude.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
//! This module provides access to items available in both the standard library
//! and the `alloc` facade crate while concealing the feature-flag based compile
//! time selection between those options.
#![allow(unused_imports)]

macro_rules! multiplex_alloc {
($($alloc: path, $std: path),*) => {
$(
#[cfg(all(feature = "alloc", not(feature = "std")))]
pub(crate) use $alloc;
#[cfg(feature = "std")]
pub(crate) use $std;
)*
};
}

macro_rules! multiplex_core {
($($core: path, $std: path),*) => {
$(
#[cfg(not(feature = "std"))]
pub(crate) use $core;
#[cfg(feature = "std")]
pub(crate) use $std;
)*
};
}

macro_rules! alloc_only {
($($alloc: path),*) => {
$(
#[cfg(all(feature = "alloc", not(feature = "std")))]
pub(crate) use $alloc;
)*
};
}

multiplex_alloc! {
alloc::borrow::Borrow, ::std::borrow::Borrow,
alloc::borrow::ToOwned, ::std::borrow::ToOwned,
alloc::boxed::Box, ::std::boxed::Box,
alloc::String, ::std::string::String,
alloc::string::ToString, ::std::string::ToString,
alloc::Vec, ::std::vec::Vec
}

multiplex_core! {
core::fmt, ::std::fmt,
core::slice, ::std::slice
}

alloc_only! {
alloc::slice::SliceConcatExt
}

2 changes: 2 additions & 0 deletions regex-syntax/src/unicode.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::cmp::Ordering;
use std::result;
use prelude::{String, ToString, Vec};

use ucd_util::{self, PropertyValues};

Expand Down Expand Up @@ -372,6 +373,7 @@ impl Iterator for AgeIter {

#[cfg(test)]
mod tests {
use std_test::prelude::v1::*;
use super::{contains_simple_case_mapping, simple_fold};

#[test]
Expand Down