config: Improve API for `DiagnosticsLogging` configuration. by mukilan · Pull Request #44703 · servo/servo

mukilan · 2026-05-04T04:40:26Z

The current extend_from_string API is not suitable for embedders as it was written specifically for servoshell and therefore makes a lot of assumptions - the binary name, the format and text of the help message, etc. It also exits the whole process after displaying the help string.

Change the behaviour of this API by making it responsible only for parsing string flags. The logic for formatting and displaying the help message is moved to servoshell. This patch also switches the DiagnosticsLogging from a struct to a BitArray and exposes strum-based APIs so that the embedder can enumerate the available diagnostics option, retrieve the documentation string associated with each option and also parse the strings back to a valid DiagnosticsLogging struct.

Testing: Includes a unit test for the API used by embedders.

mrobinson

This looks good. My main concern is the addition of 5 new crate dependencies to the servo itself. Is it possible to avoid the dependency on bitvec somehow?

mrobinson · 2026-05-05T08:25:52Z

+    #[strum(to_string = "style-tree")]
+    #[strum(message = "Log the DOM after each restyle")]


Maybe we should also include the rustdoc string so that these values also show up in the API documentation?

I think I tried that as strum also allows programmatically retrieving a variant's documentation string, but that means we can't split the documentation as message and detailed_message. But perhaps detailed_message is not very useful as only two variants have that. I'll remove detailed_message and switch message to doc strings.

I've switched to doc comments in 5d4fc99.

mrobinson · 2026-05-05T08:43:46Z

+
+    if cli_options.contains(&"help".into()) {
+        // TODO: Remove hardcoded binary name by perhaps receiving this a an argument.
+        println!("Usage: servoshell -Z option,[option,...]\n\twhere options include:");


Suggested change

println!("Usage: servoshell -Z option,[option,...]\n\twhere options include:");

println!(

"Usage: {} -Z option,[option,...]\n\twhere options include:"

env::args().first().unwrap_or_default()

);

mrobinson · 2026-05-05T08:44:03Z

+    }
+
+    if cli_options.contains(&"help".into()) {
+        // TODO: Remove hardcoded binary name by perhaps receiving this a an argument.


Suggested change

// TODO: Remove hardcoded binary name by perhaps receiving this a an argument.

// TODO: Remove hardcoded binary name by perhaps receiving this as an argument.

Though maybe you could just do this with the code below?

I didn't want to do that because the grand parent in the call chain of this function is already doing the same work to split the args and explictly passing down the args without binary. We can just thread that through the call chain, but I haven't tested what the binary name would look like in Android or OHOS, so I decided to handle it separately.

mukilan

Is it possible to avoid the dependency on bitvec somehow?

We could use a HashSet<DiagnosticLoggingOptions> instead if we are not concerned about space usage or the performance overhead during layout. Otherwise, I am not sure how to avoid the dependency without rolling our own BitArray like data structure.

mukilan · 2026-05-05T09:03:27Z

+    #[strum(to_string = "style-tree")]
+    #[strum(message = "Log the DOM after each restyle")]


I think I tried that as strum also allows programmatically retrieving a variant's documentation string, but that means we can't split the documentation as message and detailed_message. But perhaps detailed_message is not very useful as only two variants have that. I'll remove detailed_message and switch message to doc strings.

mukilan · 2026-05-05T09:12:18Z

+    }
+
+    if cli_options.contains(&"help".into()) {
+        // TODO: Remove hardcoded binary name by perhaps receiving this a an argument.


I didn't want to do that because the grand parent in the call chain of this function is already doing the same work to split the args and explictly passing down the args without binary. We can just thread that through the call chain, but I haven't tested what the binary name would look like in Android or OHOS, so I decided to handle it separately.

mukilan · 2026-05-06T04:43:29Z

Is it possible to avoid the dependency on bitvec somehow?

We could use a HashSet<DiagnosticLoggingOptions> instead if we are not concerned about space usage or the performance overhead during layout. Otherwise, I am not sure how to avoid the dependency without rolling our own BitArray like data structure.

Another option is to simply use a [bool; N] here because we know the N is small. I'm not sure if we plan to add more diagnostic options in the future, but maybe this is fine for now?

mrobinson · 2026-05-06T08:02:09Z

We could use a HashSet<DiagnosticLoggingOptions> instead if we are not concerned about space usage or the performance overhead during layout. Otherwise, I am not sure how to avoid the dependency without rolling our own BitArray like data structure.

Another option is to simply use a [bool; N] here because we know the N is small. I'm not sure if we plan to add more diagnostic options in the future, but maybe this is fine for now?

I think this is probably fine for now. Thanks!

mukilan · 2026-05-07T06:07:03Z

I've updated the PR to use [bool; N] instead of the BitArray. The new push also fixes #44689.

The current `extend_from_string` API is not suitable for embedders as it was written specifically for servoshell and therefore makes a lot of assumptions - the binary name, the format and text of the help message, etc. It also exits the whole process after displaying the help string. Change the behaviour of this API by making it responsible only for parsing string flags. The logic for formatting and displaying the help message is moved to servoshell. This patch also switches the `DiagnosticsLogging` from a struct to a `BitArray` and exposes strum-based APIs so that the embedder can enumerate the available diagnostics option, retrieve the documentation string associated with each option and also parse the strings back to a valid `DiagnosticsLogging` struct. Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>

mukilan requested review from Loirooriol, gterzian and mrobinson as code owners May 4, 2026 04:40

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 4, 2026

mrobinson requested changes May 5, 2026

View reviewed changes

servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 5, 2026

mukilan commented May 5, 2026

View reviewed changes

mukilan force-pushed the improve-diagnostics-logging-api branch from ab2f2ae to 5d4fc99 Compare May 5, 2026 10:09

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 5, 2026

mukilan force-pushed the improve-diagnostics-logging-api branch from 5d4fc99 to 53082a3 Compare May 5, 2026 12:29

mukilan force-pushed the improve-diagnostics-logging-api branch from 53082a3 to ea33c29 Compare May 7, 2026 06:05

mukilan mentioned this pull request May 7, 2026

config: remove unused mut from DiagnosticsLogging #44747

Closed

mrobinson approved these changes May 7, 2026

View reviewed changes

Comment thread components/config/opts.rs

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label May 7, 2026

mukilan force-pushed the improve-diagnostics-logging-api branch from ea33c29 to 7396780 Compare May 8, 2026 05:09

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 8, 2026

mukilan enabled auto-merge May 8, 2026 05:11

mukilan added this pull request to the merge queue May 8, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 8, 2026

Merged via the queue into servo:main with commit 662c1b6 May 8, 2026
33 checks passed

mukilan deleted the improve-diagnostics-logging-api branch May 8, 2026 05:51

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 8, 2026

mukilan mentioned this pull request May 8, 2026

"warning: variable does not need to be mutable" in release builds #44689

Closed

		#[strum(to_string = "style-tree")]
		#[strum(message = "Log the DOM after each restyle")]

	// TODO: Remove hardcoded binary name by perhaps receiving this a an argument.
	// TODO: Remove hardcoded binary name by perhaps receiving this as an argument.

Uh oh!

Uh oh!

Conversation

mukilan commented May 4, 2026

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukilan commented May 6, 2026

Uh oh!

mrobinson commented May 6, 2026

Uh oh!

mukilan commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mukilan commented May 7, 2026 •

edited

Loading