Define a public API in the base (currently "base_rs") crate. [chromium/src : main]
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
To view, visit change 3510047. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Adrian Taylor.
danakj uploaded patch set #3 to this change.
Define a public API in the base (currently "base_rs") crate.
This will allow Rust unit tests to be written in base_unittests. And
will allow other Rust code throughout Chromium to start to make use of
the base crate.
R=adet...@chromium.org
Bug: 1296156, 1293979
Change-Id: Ie3bfc13ebe09031ff08c66d374b67ac0f232ceda
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
---
M base/json/json_parser.rs
M base/json/mod.rs
M base/lib.rs
M base/rs_glue/mod.rs
M base/rs_glue/values_glue.cc
M base/rs_glue/values_glue.h
M base/values.rs
M base/values_deserialization.rs
8 files changed, 45 insertions(+), 27 deletions(-)
To view, visit change 3510047. To unsubscribe, or for help writing mail filters, visit settings.
Adrian Taylor (Gerrit)
Attention is currently required from: danakj.
Patch set 3:Code-Review +1
4 comments:
Commit Message:
Patch Set #3, Line 11:
the base crate.I am still apprehensive about exposing cross-component Rust APIs. Just due to fear of 'unknown unknowns'.
By doing this, I think you are representing that you have thought all this through more thoroughly than me, and nothing's going to go wrong :) I wonder if there are opportunities to add more test targets to build/rust/tests to explore any corner-cases, e.g. to have situations with diamond-dependencies, or some kind of Rust/C++ combination permutation we haven't tested yet.
I don't have anything specific in mind unfortunately. It's just my spidey-senses continue to tell me that this is a Big Step and I'm nervous.
Patchset:
LGTM but generally a bit nervous - see comments. A couple of nitty considerations.
File base/json/json_parser.rs:
Patch Set #3, Line 50:
pub type JsonOptions = ffi::JsonOptions;If we are exposing this as an API, _perhaps_ we should add documentation to this struct and its fields? (in the cxx::bridge above)
(I don't really mind, just trying to establish the right norms)
File base/lib.rs:
Patch Set #3, Line 10:
pub use json::json_parser::{decode_json, JsonOptions};I would argue that in Rust it's probably more normal to have deeply-nested namespaces, because it just works more fluidly than in C++, so we might want to continue to have these things in `base::json` or similar? I don't mind on this specific case but if these are the first cross-component Rust APIs we're exposing, we should come up with good norms.
To view, visit change 3510047. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patch set 4:Commit-Queue +2
danakj (Gerrit)
3 comments:
Commit Message:
Patch Set #3, Line 11:
the base crate.
I am still apprehensive about exposing cross-component Rust APIs. […]
Finding unknown unknowns is a goal of doing this - like the buildflag and IOS stuff I encountered when trying to move the tests into base_unittests. It's likely we'll find more, and file bugs as we go.
The linking model at least seems sound though, once https://bugs.chromium.org/p/gn/issues/detail?id=276 is addressed (and the whole rlib vs .o thing). In non-component build, for linking, there's no problems that I see at the moment.
Uncomfortable excitement :)
File base/json/json_parser.rs:
Patch Set #3, Line 50:
pub type JsonOptions = ffi::JsonOptions;
If we are exposing this as an API, _perhaps_ we should add documentation to this struct and its fiel […]
Done.
In the process, I added a with_chromium_extensions(max_size: usize) to the type and documented it and tested it as well.
File base/lib.rs:
Patch Set #3, Line 10:
pub use json::json_parser::{decode_json, JsonOptions};
I would argue that in Rust it's probably more normal to have deeply-nested namespaces, because it ju […]
Yeah, I've been reading on this a bit via API guidance and books etc. We can move it into a submodule later if we need to, but I think we can roughly model our Rust public APIs after the C++ namespaces for now.
If there was no need to have base::foo:: in C++ then there's little reason in Rust - and it makes things harder to find.
Inside the crate, the full path would make sense to use however. The Rust for Rustaceans book makes a point to separate "lots of modules for developers of your crate" vs "a clear and simple public API for developers using your crate" and leans toward fewer modules when reasonable to do so.
As the json module would have a single function, I see no strong reason to make the module itself part of the public API.
To view, visit change 3510047. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/json/json_reader.h
Insertions: 3, Deletions: 0.
@@ -76,6 +76,9 @@
// This parser historically accepted, without configuration flags,
// non-standard JSON extensions. This flag enables that traditional parsing
// behavior.
+ //
+ // This set of options is mirrored in Rust
+ // base::JsonOptions::with_chromium_extensions().
JSON_PARSE_CHROMIUM_EXTENSIONS = JSON_ALLOW_COMMENTS |
JSON_ALLOW_CONTROL_CHARS |
JSON_ALLOW_VERT_TAB | JSON_ALLOW_X_ESCAPES,
```
```
The name of the file: base/json/json_parser.rs
Insertions: 45, Deletions: 0.
@@ -18,14 +18,27 @@
#[cxx::bridge(namespace=base::rs_glue)]
mod ffi {
+ /// Options for parsing JSON inputs. Largely a mirror of the C++ `base::JSONParserOptions`
+ /// bitflags, represented as a friendlier struct-of-bools instead.
#[namespace=base::ffi::json::json_parser]
struct JsonOptions {
+ /// Allows commas to exist after the last element in structures.
allow_trailing_commas: bool,
+ /// If set the parser replaces invalid code points (i.e. lone surrogates) with the Unicode
+ /// replacement character (U+FFFD). If not set, invalid code points trigger a hard error and
+ /// parsing fails.
replace_invalid_characters: bool,
+ /// Allows both C (/* */) and C++ (//) style comments.
allow_comments: bool,
+ /// Permits unescaped ASCII control characters (such as unescaped \r and \n) in the range
+ /// [0x00,0x1F].
allow_control_chars: bool,
+ /// Permits \\v vertical tab escapes.
allow_vert_tab: bool,
+ /// Permits \\xNN escapes as described above.
allow_x_escapes: bool,
+ /// The maximum recursion depth to walk while parsing nested JSON objects. JSON beyond the
+ /// specified depth will be ignored.
max_depth: usize,
}
@@ -48,6 +61,26 @@
}
pub type JsonOptions = ffi::JsonOptions;
+impl JsonOptions {
+ /// Construct a JsonOptions with common Chromium extensions.
+ ///
+ /// Per base::JSONParserOptions::JSON_PARSE_CHROMIUM_EXTENSIONS:
+ ///
+ /// This parser historically accepted, without configuration flags,
+ /// non-standard JSON extensions. This enables that traditional parsing
+ /// behavior.
+ pub fn with_chromium_extensions(max_depth: usize) -> JsonOptions {
+ JsonOptions {
+ allow_trailing_commas: false,
+ replace_invalid_characters: false,
+ allow_comments: true,
+ allow_control_chars: true,
+ allow_vert_tab: true,
+ allow_x_escapes: true,
+ max_depth,
+ }
+ }
+}
/// Decode some JSON into C++ base::Value object tree.
///
@@ -148,6 +181,18 @@
use crate::values::ValueSlotRef;
#[test]
+ fn test_chromium_extensions() {
+ let opts = super::JsonOptions::with_chromium_extensions(101);
+ assert_eq!(opts.allow_trailing_commas, false);
+ assert_eq!(opts.replace_invalid_characters, false);
+ assert_eq!(opts.allow_comments, true);
+ assert_eq!(opts.allow_control_chars, true);
+ assert_eq!(opts.allow_vert_tab, true);
+ assert_eq!(opts.allow_x_escapes, true);
+ assert_eq!(opts.max_depth, 101);
+ }
+
+ #[test]
fn test_decode_json() {
// Exhaustively tested by existing C++ JSON tests.
// This test is almost pointless but it seems wise to have a single
```
Define a public API in the base (currently "base_rs") crate.
This will allow Rust unit tests to be written in base_unittests. And
will allow other Rust code throughout Chromium to start to make use of
the base crate.
R=adet...@chromium.org
Bug: 1296156, 1293979
Change-Id: Ie3bfc13ebe09031ff08c66d374b67ac0f232ceda
Cq-Include-Trybots: luci.chromium.try:android-rust-arm-rel,linux-rust-x64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3510047
Reviewed-by: Adrian Taylor <adet...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#978851}
---
M base/json/json_parser.rs
M base/json/json_reader.h
M base/json/mod.rs
M base/lib.rs
M base/rs_glue/mod.rs
M base/rs_glue/values_glue.cc
M base/rs_glue/values_glue.h
M base/values.rs
M base/values_deserialization.rs
9 files changed, 97 insertions(+), 27 deletions(-)