Cleanup str(n)cmp with std::string_view #4567 by sivansh11 · Pull Request #4742 · canonical/mir

sivansh11 · 2026-03-04T15:21:07Z

replaces error prone str(n)cmp with more expressive std::string_view

sivansh11 · 2026-03-04T15:24:53Z

@tarek-y-ismail, I am not able to test the changes, since ctest is getting stuck on the 6th test (named wlcs)
the test is getting stuck even before my current commit

Copilot

Pull request overview

This PR modernizes C-string comparisons across Mir by replacing strcmp/strncmp usages with clearer std::string_view equality and starts_with() checks, aligning with #4567’s guidance.

Changes:

Replaced strcmp equality/inequality checks with std::string_view comparisons (often using "..."sv literals).
Replaced common strncmp(prefix, ..., N) patterns with std::string_view::starts_with().
Added <string_view> includes (and std::string_view_literals where needed) in many touched files.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit-tests/input/evdev/test_evdev_input_platform.cpp	Switches devnode matching from `strcmp` to `std::string_view` compare (currently missing `<string_view>` include).
tests/unit-tests/console/test_minimal_console_services.cpp	Replaces `/dev/` + `/sys` comparisons with `starts_with()` / `std::string_view` equality; adds `<string_view>`.
tests/unit-tests/console/test_linux_virtual_terminal.cpp	Replaces `/dev/` prefix check with `starts_with()`; adds `<string_view>`.
tests/miral/wayland_extensions.cpp	Replaces Wayland interface name `strcmp` checks with `std::string_view` comparisons; adds `<string_view>`.
tests/miral/external_client.cpp	Replaces getenv/`strcmp` check with safe pointer check + string_view literal compare; adds `<cstdlib>`, `<string_view>`, and literals.
tests/mir_test_framework/test_wlcs_display_server.cpp	Replaces Wayland resource class `strcmp` checks with string_view/literal comparisons; adds `<string_view>` and literals.
tests/mir_test_doubles/mock_drm.cpp	Replaces `/dev/dri/` prefix `strncmp` with `starts_with()`; adds `<string_view>`.
src/server/scene/basic_surface.cpp	Replaces `strcmp` on pthread name with string_view literal compare; adds `<string_view>` and literals.
src/server/graphics/platform_probe.cpp	Replaces `strcmp` ordering / module name checks with `std::string_view` comparisons; adds `<string_view>` and literals.
src/server/console/minimal_console_services.cpp	Replaces `DEVNAME=` `strncmp` parsing with `starts_with()`; adds `<string_view>`.
src/server/console/logind_console_services.cpp	Replaces several `strcmp`/`strncmp` checks with string_view equality/`starts_with()`; adds `<string_view>` and literals.
src/server/console/linux_virtual_terminal.cpp	Replaces `DEVNAME=` `strncmp` parsing with `starts_with()`; adds `<string_view>`.
src/platforms/wayland/displayclient.cpp	Replaces Wayland global interface `strcmp` checks with string_view/literal comparisons; adds `<string_view>` and literals.
src/platforms/gbm-kms/server/kms/real_kms_output.cpp	Replaces DRM property/mode `strcmp` checks with string_view literal comparisons; adds `<string_view>` and literals.
src/platforms/gbm-kms/server/kms/platform_symbols.cpp	Replaces driver string comparisons and prefix checks with string_view compares/`starts_with()`; adds `<string_view>` and literals.
src/platforms/gbm-kms/server/kms/display.cpp	Replaces devnode `strcmp` checks with `std::string_view` equality; adds `<string_view>`.
src/platforms/evdev/platform.cpp	Replaces `strncmp`/`strcmp` checks with `starts_with()` / `std::string_view` equality; adds `<string_view>`.
src/platforms/eglstream-kms/server/egl_output.cpp	Replaces DRM property `strcmp` check with string_view literal compare; adds `<string_view>` and literals.
src/platforms/atomic-kms/server/kms/display.cpp	Replaces devnode `strcmp` checks with `std::string_view` equality; adds `<string_view>`.
src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp	Replaces mode `strcmp` check with string_view literal compare; adds `<string_view>` and literals.
src/platform/udev/udev_wrapper.cpp	Replaces devpath equality and action string comparisons with `std::string_view` compares/literals; adds `<string_view>` and literals.
src/platform/options/default_configuration.cpp	Replaces library name `strcmp` ordering check with `std::string_view` ordering; adds `<string_view>` and literals.
src/miral/wayland_app.cpp	Replaces Wayland registry interface `strcmp` checks with string_view literal comparisons; adds `<string_view>` and literals.
src/miral/launch_app.cpp	Replaces env-var prefix `strncmp` checks with `starts_with()`; adds `<string_view>`.
include/test/mir/test/event_matchers.h	Replaces matcher `strcmp` with `std::string_view` inequality; adds `<string_view>`.
examples/miral-shell/shell_main.cpp	Replaces `strcmp` against env-var values with string_view literal comparisons; includes `<string_view>` and literals.
examples/example-server-lib/wayland_app.cpp	Replaces Wayland registry interface `strcmp` checks with string_view literal comparisons; includes `<string_view>` and literals.
examples/client/mir_shell_demo.cpp	Replaces registry `strcmp` checks with `"..."sv` and `std::string_view` compares (currently missing `<string_view>` + literals import).

Copilot · 2026-03-04T17:11:14Z

+    if (interface == "wl_compositor"sv)
    {
        globals::compositor = static_cast<wl_compositor*>(wl_registry_bind(
            registry, id, &wl_compositor_interface, std::min(version, 3u)));
    }
-    else if (strcmp(interface, "wl_shm") == 0)
+    else if (interface == "wl_shm"sv)
    {


This block uses the "..."sv string_view literal and std::string_view{...} but the file doesn't include <string_view> nor import std::string_view_literals, so it will not compile. Add #include <string_view> and using namespace std::string_view_literals; (or avoid sv by comparing via std::string_view{interface} == "wl_compositor").

This block uses the "..."sv string_view literal and std::string_view{...} but the file doesn't include <string_view> nor import std::string_view_literals, so it will not compile. Add #include <string_view> and using namespace std::string_view_literals; (or avoid sv by comparing via std::string_view{interface} == "wl_compositor").

This only compiles because <string> transitively includes <string_view> in the current libstcc++ implementation. (Which then means using namespace std::literals pulls in string_view_literals.)

There's no guarantee this will work with other library implementations (nor future versions of this one).

AlanGriffiths · 2026-03-11T10:29:07Z

    {
        // Starting an X server on LP builder, or Fedora CI, doesn't work
-        auto const lp_fake_runtime_dir = strcmp(getenv("XDG_RUNTIME_DIR"), "/tmp") == 0;
+        auto const xdg_dir = getenv("XDG_RUNTIME_DIR");


There is a subtle change here: the original code has undefined behaviour (probably a SIGSEGV) if getenv("XDG_RUNTIME_DIR") returns nullptr. The new code sets lp_fake_runtime_dir to false and carries on.

It would probably be better to abort() if there is no XDG_RUNTIME_DIR in the environment.

However, there are checks elsewhere that ensure XDG_RUNTIME_DIR is set, so this is likely moot.

@AlanGriffiths should I add the abort check here even tho it is rechecked later on ? Or maybe just assert ?

@sivansh11 in general, it is better to fail noisily (i.e. abort()) when things are wrong than to carry on (set lp_fake_runtime_dir to false, assert() or "undefined behaviour").

AlanGriffiths · 2026-03-11T10:30:54Z

+    if (interface == "wl_compositor"sv)
    {
        globals::compositor = static_cast<wl_compositor*>(wl_registry_bind(
            registry, id, &wl_compositor_interface, std::min(version, 3u)));
    }
-    else if (strcmp(interface, "wl_shm") == 0)
+    else if (interface == "wl_shm"sv)
    {


AlanGriffiths · 2026-03-18T10:39:41Z

+        if (!xdg_dir)
+            std::runtime_error("XDG_RUNTIME_DIR not found in environment");


What is the intention here? This constructs an unnamed runtime_error instance (and discards it): that doesn't achieve anything.

I forgot to throw, force pushed a fix to this

if user does not have XDG_RUNTIME_DIR, error out

AlanGriffiths · 2026-03-24T12:12:15Z

+#include <string_view>

 namespace mo = mir::options;
+using namespace std::string_view_literals;


sivansh11 · 2026-03-24T14:08:40Z

    auto const rhs_desc = rhs->load_function<mir::graphics::DescribeModule>("describe_graphics_module", MIR_SERVER_GRAPHICS_PLATFORM_VERSION);

-    return strcmp(rhs_desc()->name, lhs_desc()->name) > 0;
+    return std::string_view{rhs_desc()->name} > std::string_view{lhs_desc()->name};


It is used here @AlanGriffiths

Where are you using a literal?

ah right, I missed it

AlanGriffiths · 2026-03-25T09:18:20Z

+        if (!xdg_dir)
+            throw std::runtime_error("XDG_RUNTIME_DIR not found in environment");


Throwing an exception likely results in an unhelpful message from the test framework:

Suggested change

if (!xdg_dir)

throw std::runtime_error("XDG_RUNTIME_DIR not found in environment");

if (!xdg_dir)

{

puts("This test expects XDG_RUNTIME_DIR to be set");

abort();

}

AlanGriffiths · 2026-03-25T09:20:53Z

+    if (interface == "wl_compositor"sv)
    {
        globals::compositor = static_cast<wl_compositor*>(wl_registry_bind(
            registry, id, &wl_compositor_interface, std::min(version, 3u)));
    }
-    else if (strcmp(interface, "wl_shm") == 0)
+    else if (interface == "wl_shm"sv)
    {


This block uses the "..."sv string_view literal and std::string_view{...} but the file doesn't include <string_view> nor import std::string_view_literals, so it will not compile. Add #include <string_view> and using namespace std::string_view_literals; (or avoid sv by comparing via std::string_view{interface} == "wl_compositor").

This only compiles because <string> transitively includes <string_view> in the current libstcc++ implementation. (Which then means using namespace std::literals pulls in string_view_literals.)

There's no guarantee this will work with other library implementations (nor future versions of this one).

AlanGriffiths · 2026-04-07T08:44:43Z

@sivansh11 have you abandoned this? You've not responded to review comments for a couple of weeks:

Closing for now: feel free to reopen if you can address the above.

sivansh11 requested a review from a team as a code owner March 4, 2026 15:21

Saviq requested a review from Copilot March 4, 2026 17:07

Copilot started reviewing on behalf of Saviq March 4, 2026 17:08 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

AlanGriffiths requested changes Mar 11, 2026

View reviewed changes

AlanGriffiths reviewed Mar 18, 2026

View reviewed changes

sivansh11 force-pushed the fix/4567 branch from e68daa3 to a5c1060 Compare March 18, 2026 10:45

sivansh11 added 2 commits March 24, 2026 11:56

Cleanup str(n)cmp with std::string_view canonical#4567

d95c0a2

Fixed potential undefined behaviour with getenv

9084515

if user does not have XDG_RUNTIME_DIR, error out

Saviq force-pushed the fix/4567 branch from a5c1060 to 9084515 Compare March 24, 2026 10:56

AlanGriffiths requested changes Mar 24, 2026

View reviewed changes

sivansh11 commented Mar 24, 2026

View reviewed changes

sivansh11 and others added 2 commits March 24, 2026 19:50

Merge branch 'canonical:main' into fix/4567

f512e04

Added missing #include <string_view>

119a894

AlanGriffiths requested changes Mar 25, 2026

View reviewed changes

AlanGriffiths closed this Apr 7, 2026

		if (!xdg_dir)
		std::runtime_error("XDG_RUNTIME_DIR not found in environment");

		if (!xdg_dir)
		throw std::runtime_error("XDG_RUNTIME_DIR not found in environment");

Conversation

sivansh11 commented Mar 4, 2026

Uh oh!

sivansh11 commented Mar 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

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!

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!

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!

sivansh11 Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanGriffiths commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sivansh11 Mar 24, 2026 •

edited

Loading