Fix memory leak, and many lint warning. #602

hysw · 2025-11-26T23:01:03Z

Fix two memory leak, and many lint warning.

Created a tiny Guidelines Support Library
Add some helper function to make sure we do qt ownership correctly.

angela28chen

Thanks for making this effort to make Dive ui code use "new" in a more intentioned way

angela28chen · 2025-11-28T17:02:33Z

src/dive/ui/forward.h


 // Forward declarations for UI.

+class OverlayHelper;


this seems repeated (line 74)

Eh, I probably selected the wrong range when running sort+deduplicate in the editor.

angela28chen · 2025-11-28T17:08:31Z

src/dive/ui/lint.h

+    return ptr;  // NOLINT
+}
+
+// Create a new Qt object.


I like the examples section for how to create new layouts using LayoutHelper. Can we have a similar brief example section for QtNew and QtNewUnowned?

They are just new with static_assert.
Will add a bit more document.

angela28chen · 2025-11-28T17:13:46Z

ui/analyze_window.cpp

+#include "ui/overlay.h"
+#include "ui/settings.h"
+
+using DiveLint::QtNew;


Is there any downside to avoiding "using" and just calling the full DiveLint::QtNew() when using this or is it just wordy? I slightly prefer the wordy version since I think it makes it clearer that QtNew() isn't something defined by Qt despite the prefix

Though I guess it might be confusing to see the word "Lint" in there. Could it not just be Dive::QtNew() or DiveQt::QtNew or something?

Those are annotations, and more specifically lint annotations. They don't do anything other than doing additional lint checks.

Also regarding to using alias https://google.github.io/styleguide/cppguide.html#Aliases

hysw · 2025-11-28T19:50:19Z

Thanks for making this effort to make Dive ui code use "new" in a more intentioned way

Had a bit offline chat earlier with @footballhead , instead of disable lint check for pointer ownership, it seems sensible to explicitly annotate allocations.

footballhead · 2025-11-26T23:17:42Z

src/dive/lint/gsl.h

+namespace gsl
+{
+
+template<class T, std::enable_if_t<std::is_pointer_v<T>, bool> = true>  //


why the trailing //?

We probably wan to remove clang-format rule AlwaysBreakTemplateDeclarations: MultiLine

footballhead · 2025-12-01T15:37:19Z

.clang-tidy

  - "-cppcoreguidelines-avoid-non-const-global-variables" # ABSL_FLAG
  - "-cppcoreguidelines-use-enum-class"
  - "-readability-redundant-access-specifiers" # qt
+  - "-cppcoreguidelines-pro-bounds-avoid-unchecked-container-access" # https://abseil.io/tips/224


+1 love adding the justification in the comment

src/dive/ui/layout_helper.h

src/dive/lint/gsl.h

footballhead · 2025-12-01T15:56:38Z

ui/analyze_window.cpp

+using DiveLint::QtNew;
+using DiveLint::QtNewUnowned;


nit, likely not an issue here since the using isn't inside the namespace but (in general) prefer to fully qualify https://abseil.io/tips/119

Suggested change

using DiveLint::QtNew;

using DiveLint::QtNewUnowned;

using ::DiveLint::QtNew;

using ::DiveLint::QtNewUnowned;

(https://abseil.io/tips/119 also recommends putting this in a namespace but I can see that there isn't one here :)

footballhead · 2025-12-01T15:58:00Z

ui/analyze_window.cpp

    m_available_metrics(available_metrics)
 {
    qDebug() << "AnalyzeDialog created.";
+    InitializeLayout();


Is it worth documenting somewhere that this is called from the constructor so should not be made virtual?

It should be a factory function

auto Create() { auto dialog = new AnalyzeDialog; if (!AnalyzeDialog->InitializeLayout()) { delete dialog; return nullptr; } return dialog; }

however the rest of the code are not capable of handling creation failure at the moment.

Until that happens, document that this is called from the constructor so should not be made virtual?

footballhead · 2025-12-01T16:00:21Z

ui/analyze_window.cpp


    // Left Panel Layout
-    m_left_panel_layout = new QVBoxLayout();
+    m_left_panel_layout = QtNewUnowned<QVBoxLayout>();


Should we look at using the layout helper here as well?

Yes. Although that's a lot of reordering probably fitted for the next PR.

I intentionally did not add AddWidget and AddLayout since they encourage having unparented elements.

src/.clang-format

footballhead · 2025-12-01T19:04:43Z

src/dive/ui/layout_helper.h

+//   auto* label = layout.New<QLabel>(tr("text"));
+//
+// Add a new item to an existing layout:
+//   auto* label = NewWidgetLayout{layout}.New<QLabel>(tr("text"));


nit: since NewWidgetLayout is a funtion call, should the {} be () instead?

Suggested change

// auto* label = NewWidgetLayout{layout}.New<QLabel>(tr("text"));

// auto* label = NewWidgetLayout(layout).New<QLabel>(tr("text"));

src/dive/ui/layout_helper.h

src/dive/ui/utils/layout_helper.h

src/dive/ui/layout_helper.h

footballhead · 2025-12-01T19:15:45Z

ui/analyze_window.cpp

    m_available_metrics(available_metrics)
 {
    qDebug() << "AnalyzeDialog created.";
+    InitializeLayout();


Until that happens, document that this is called from the constructor so should not be made virtual?

- Created a tiny Guidelines Support Library - Add some helper function to make sure we do qt ownership correctly.

hysw force-pushed the lint-1126 branch from 42bc54d to c540547 Compare November 26, 2025 23:02

hysw requested review from GrantComm, angela28chen, footballhead and wangra-google November 26, 2025 23:05

hysw force-pushed the lint-1126 branch 5 times, most recently from 6e3c7d5 to e790495 Compare November 27, 2025 00:59

wangra-google previously approved these changes Nov 27, 2025

View reviewed changes

angela28chen reviewed Nov 28, 2025

View reviewed changes

footballhead reviewed Dec 1, 2025

View reviewed changes

hysw dismissed wangra-google’s stale review via 7e0dca0 December 1, 2025 17:16

hysw force-pushed the lint-1126 branch 2 times, most recently from a3afd32 to a73af62 Compare December 1, 2025 18:10

footballhead reviewed Dec 1, 2025

View reviewed changes

wangra-google previously approved these changes Dec 1, 2025

View reviewed changes

hysw dismissed wangra-google’s stale review via dd512fb December 26, 2025 00:06

hysw force-pushed the lint-1126 branch 3 times, most recently from 6b4fffe to c4ca939 Compare December 26, 2025 00:27

hysw marked this pull request as draft December 26, 2025 00:28

Componentize DiveUI (part 1).

d28316f

hysw force-pushed the lint-1126 branch 2 times, most recently from 4920263 to ffcb8a7 Compare December 26, 2025 00:48

Fix two memory leak, and many lint warning.

2cc46c6

- Created a tiny Guidelines Support Library - Add some helper function to make sure we do qt ownership correctly.

hysw force-pushed the lint-1126 branch from ffcb8a7 to 2cc46c6 Compare December 26, 2025 00:50

	// auto* label = NewWidgetLayout{layout}.New<QLabel>(tr("text"));
	// auto* label = NewWidgetLayout(layout).New<QLabel>(tr("text"));

Fix memory leak, and many lint warning. #602

Are you sure you want to change the base?

Fix memory leak, and many lint warning. #602

Uh oh!

Conversation

hysw commented Nov 26, 2025

Uh oh!

angela28chen 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!

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!

hysw commented Nov 28, 2025

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!

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!

footballhead Dec 1, 2025 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

footballhead Dec 1, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

footballhead Dec 1, 2025 •

edited

Loading

footballhead Dec 1, 2025 •

edited

Loading