Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 5, 2025

Related: #4503

What's new?

Fixes TICS static analysis violations across the codebase:

Fixed violations:

  1. COV_CPP_USE_AFTER_MOVE in alarm_factory.cpp:35: Changed cb = std::move(callback) to cb = callback to avoid the analyzer warning about using std::move on a const reference parameter.

  2. COV_CPP_UNINIT in basic_decoration.cpp:124: Reordered the buffer_format member declaration in the BufferStreams class to appear before the buffer stream members, ensuring proper initialization order.

  3. COV_CPP_USE_AFTER_MOVE in emitter.cpp:116: Removed std::move from const& parameter (same pattern as alarm_factory.cpp).

  4. COV_CPP_MISMATCHED_ITERATOR in client.cpp:46: Changed from begin(*map)/end(*map) to map->begin()/map->end() for consistent iterator usage.

  5. COV_CPP_UNINIT in display.cpp:420: Initialize glm::mat2 transformation to identity matrix {1}.

  6. COV_CPP_RESOURCE_LEAK in desktop_file_manager.cpp:265: Added g_autofree to prevent memory leak of sandboxed_app_id.

  7. COV_CPP_RESOURCE_LEAK in egl_output.cpp:86: Store mmap pointer and size as members, call munmap in destructor.

  8. COV_CPP_UNINIT in input_method_v1.cpp:453,496,543: Changed T value_; to T value_{}; in optional_value.h to value-initialize the member.

False positives (no changes needed):

  • basic_decoration.cpp:152, desktop_file_manager.cpp:109, foreign_toplevel_manager_v1.cpp:735,781,792: The weak_ptr::lock() result is correctly stored in the shared_ptr member.
  • buffer_allocator.cpp:119: Uses member stream, not the moved-from parameter.
  • buffer_allocator.cpp:192: Memory is freed by wl_resource destroy listener callback.
  • cursor.cpp:111: data_buffer doesn't own memory; the shm_pool does.
  • config_file.cpp:208: The inotify API guarantees event.name is null-terminated.

How to test

  • Review the code changes to verify the fixes
  • CI should pass with the updated code

Checklist

  • Tests added and pass
  • Adequate documentation added
  • (optional) Added Screenshots or videos
Original prompt

This section details on the original issue you should resolve

<issue_title>Top 20 TICS violations</issue_title>
<issue_description>Another set of violations TICS is reporting.

Annotation Type Rule Level Impact at project Impact at scope Message
alarm_factory.cpp:35 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% Returning while reference parameter "callback" is in a moved state.
basic_decoration.cpp:124 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "this->buffer_format" when calling "create_buffer_stream".
basic_decoration.cpp:152 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "this->session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
buffer_allocator.cpp:119 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% "stream" is used after it has been already moved.
buffer_allocator.cpp:192 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "me" going out of scope leaks the storage it points to.
client.cpp:46 AI COV_CPP_MISMATCHED_ITERATOR 1 0.02% 0.02% Using iterator "std::end(*map)" from "*map" with "map".
client.cpp:46 AI COV_CPP_MISMATCHED_ITERATOR 1 0.02% 0.02% Using iterator "std::remove_if(std::begin(*map), std::end(*map), mir::wayland::Client::unregister_client(wl_client *)::lambda(T1 &) (instance 1))" from "*map" with "map".
config_file.cpp:208 AI COV_CPP_STRING_NULL 1 0.02% 0.02% Passing unterminated string "event.name" to "path", which expects a null-terminated string.
cursor.cpp:111 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "data_buffer" going out of scope leaks the storage it points to.
desktop_file_manager.cpp:109 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
desktop_file_manager.cpp:265 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "sandboxed_app_id" going out of scope leaks the storage it points to.
display.cpp:420 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "transformation" when calling "set_transformation".
egl_output.cpp:86 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "map" going out of scope leaks the storage it points to.
emitter.cpp:116 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% Returning while reference parameter "child" is in a moved state.
foreign_toplevel_manager_v1.cpp:735 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
foreign_toplevel_manager_v1.cpp:781 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
foreign_toplevel_manager_v1.cpp:792 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
input_method_v1.cpp:453 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.pixel_format". Field "spec.pixel_format.value_" is uninitialized when calling "apply_spec".
input_method_v1.cpp:496 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.pixel_format". Field "spec.pixel_format.value_" is uninitialized when calling "apply_spec".
input_method_v1.cpp:543 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.attached_edges". Field "spec.attached_edges.value_" is uninitialized when calling "apply_spec".

</issue_description>

<agent_instructions>Please fix the violations as listed in the table</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Saviq
Copy link
Contributor

Saviq commented Dec 5, 2025

@copilot try again

- alarm_factory.cpp: Remove std::move from const reference parameter to
  avoid use-after-move warning (COV_CPP_USE_AFTER_MOVE)
- basic_decoration.cpp: Reorder buffer_format member declaration to appear
  before buffer stream members, ensuring it's initialized before
  create_buffer_stream() uses it (COV_CPP_UNINIT)

Co-authored-by: Saviq <[email protected]>
Copy link
Contributor Author

Copilot AI commented Dec 5, 2025

@copilot try again

Fixed in 25054f1:

  1. alarm_factory.cpp:35 (COV_CPP_USE_AFTER_MOVE): Removed std::move from const reference - since callback is const&, using explicit copy makes intent clearer.

  2. basic_decoration.cpp:124 (COV_CPP_UNINIT): Moved buffer_format member declaration before buffer stream members so it's initialized before create_buffer_stream() uses it.

  3. basic_decoration.cpp:152 (COV_CPP_RETURN_LOCAL): This is a false positive - the code correctly stores weak_ptr::lock() result into a shared_ptr member. No changes needed.

Copilot AI changed the title [WIP] Fix top 20 TICS violations identified Fix TICS violations: use-after-move and uninitialized member access Dec 5, 2025
@Saviq
Copy link
Contributor

Saviq commented Dec 5, 2025

@copilot there's more cases in the original bug:

Annotation Type Rule Level Impact at project Impact at scope Message
alarm_factory.cpp:35 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% Returning while reference parameter "callback" is in a moved state.
basic_decoration.cpp:124 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "this->buffer_format" when calling "create_buffer_stream".
basic_decoration.cpp:152 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "this->session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
buffer_allocator.cpp:119 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% "stream" is used after it has been already moved.
buffer_allocator.cpp:192 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "me" going out of scope leaks the storage it points to.
client.cpp:46 AI COV_CPP_MISMATCHED_ITERATOR 1 0.02% 0.02% Using iterator "std::end(*map)" from "*map" with "map".
client.cpp:46 AI COV_CPP_MISMATCHED_ITERATOR 1 0.02% 0.02% Using iterator "std::remove_if(std::begin(*map), std::end(*map), mir::wayland::Client::unregister_client(wl_client *)::lambda(T1 &) (instance 1))" from "*map" with "map".
config_file.cpp:208 AI COV_CPP_STRING_NULL 1 0.02% 0.02% Passing unterminated string "event.name" to "path", which expects a null-terminated string.
cursor.cpp:111 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "data_buffer" going out of scope leaks the storage it points to.
desktop_file_manager.cpp:109 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
desktop_file_manager.cpp:265 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "sandboxed_app_id" going out of scope leaks the storage it points to.
display.cpp:420 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "transformation" when calling "set_transformation".
egl_output.cpp:86 AI COV_CPP_RESOURCE_LEAK 1 0.02% 0.02% Variable "map" going out of scope leaks the storage it points to.
emitter.cpp:116 AI COV_CPP_USE_AFTER_MOVE 1 0.02% 0.02% Returning while reference parameter "child" is in a moved state.
foreign_toplevel_manager_v1.cpp:735 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
foreign_toplevel_manager_v1.cpp:781 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
foreign_toplevel_manager_v1.cpp:792 AI COV_CPP_RETURN_LOCAL 1 0.02% 0.02% Using "session", which points to an out-of-scope temporary variable of type "std::weak_ptrmir::scene::Session".
input_method_v1.cpp:453 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.pixel_format". Field "spec.pixel_format.value_" is uninitialized when calling "apply_spec".
input_method_v1.cpp:496 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.pixel_format". Field "spec.pixel_format.value_" is uninitialized when calling "apply_spec".
input_method_v1.cpp:543 AI COV_CPP_UNINIT 1 0.02% 0.02% Using uninitialized value "spec.attached_edges". Field "spec.attached_edges.value_" is uninitialized when calling "apply_spec".

- emitter.cpp:116: Remove std::move from const& parameter
- client.cpp:46: Fix mismatched iterator by using member functions consistently
- display.cpp:420: Initialize transformation with identity matrix
- desktop_file_manager.cpp:265: Fix memory leak with g_autofree
- egl_output.cpp:86: Fix mmap leak by adding munmap in destructor
- optional_value.h: Value-initialize the value_ member to fix
  uninitialized value warnings in input_method_v1.cpp

Co-authored-by: Saviq <[email protected]>
Copy link
Contributor Author

Copilot AI commented Dec 5, 2025

@copilot there's more cases in the original bug:

Annotation | Type | Rule | Level | Impact at project | Impact at scope | Message
...

Fixed in ba2b319. I've addressed the full list of 20 violations:

Fixed:

  • alarm_factory.cpp:35 - Removed std::move from const&
  • basic_decoration.cpp:124 - Reordered buffer_format member
  • emitter.cpp:116 - Removed std::move from const&
  • client.cpp:46 - Use consistent iterator methods
  • display.cpp:420 - Initialize transformation to identity
  • desktop_file_manager.cpp:265 - Added g_autofree
  • egl_output.cpp:86 - Added munmap in destructor
  • input_method_v1.cpp:453,496,543 - Value-initialize optional_value

False positives (no changes needed):

  • weak_ptr::lock() patterns (basic_decoration:152, desktop_file_manager:109, foreign_toplevel:735,781,792)
  • buffer_allocator.cpp:119 (uses member, not parameter)
  • buffer_allocator.cpp:192 (freed by destroy listener)
  • cursor.cpp:111 (data_buffer doesn't own memory)
  • config_file.cpp:208 (inotify guarantees null termination)

Copilot AI changed the title Fix TICS violations: use-after-move and uninitialized member access Fix top 20 TICS violations Dec 5, 2025
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

TICS Quality Gate

✔️ Passed

mir

Coding Standards: ✔️ Passed

✔️ Condition “No new Coding Standard Violations for level 1, 2, 3 with respect to Previous analysis” passed.

See the results in the TICS Viewer

The following files have been checked for this project
  • include/core/mir/optional_value.h
  • src/platforms/eglstream-kms/server/egl_output.cpp
  • src/platforms/gbm-kms/server/kms/display.cpp
  • src/server/frontend_wayland/desktop_file_manager.cpp
  • src/server/shell/decoration/basic_decoration.cpp
  • src/server/time/alarm_factory.cpp
  • src/wayland/client.cpp
  • src/wayland/generator/emitter.cpp

TICS / TICS / Run TICS analysis

Comment on lines -46 to +50
map->erase(remove_if(
begin(*map),
end(*map),
map->erase(std::remove_if(
map->begin(),
map->end(),
[&](auto& item){ return item.first == raw; }),
end(*map));
map->end());
Copy link
Contributor

@AlanGriffiths AlanGriffiths Dec 5, 2025

Choose a reason for hiding this comment

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

OK, assuming that begin()/end() are resolved to the ones from std that changes nothing. Does it really fix a TICS violation?

I suspect the violation comes from elsewhere: not "knowing" that operator*() returns a reference to the same container. (It isn't clear that switching to operator->() should be any better.)

We can do better since C++20...

    std::erase_if(*map, [&](auto const& item){ return item.first == raw; });

@Saviq Saviq force-pushed the main branch 4 times, most recently from 39740d9 to 43ac49a Compare December 15, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Top 20 TICS violations

3 participants