microsoft / MIEngine

The Visual Studio MI Debug Engine ("MIEngine") provides an open-source Visual Studio Debugger extension that works with MI-enabled debuggers such as gdb and lldb.
MIT License
818 stars 218 forks source link

User Story 1565289: [MIEngine Natvis] Support multiple natvis files in visualizerFile launch configuration #1381

Closed gc46 closed 1 year ago

gregg-miskelly commented 1 year ago
        }

I think you want to remove the set and backing field and just have:

        /// <summary>
        /// Collection of natvis files to use when evaluating
        /// </summary>
        public List<string> VisualizerFile { get; } = new List<string>();

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad92d55785042d3851a4d472b8fbf985902, deletion_comment = False)

gc46 commented 1 year ago
        }

I think you want to remove the set and backing field and just have:

        /// <summary>
        /// Collection of natvis files to use when evaluating
        /// </summary>
        public List<string> VisualizerFile { get; } = new List<string>();

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

Why do we not want the set()?

gregg-miskelly commented 1 year ago

Why do we not want the set()?

Generally, properties with a collection type shouldn't have a set method. Instead, they get assigned in the constructor and items are added to them.

gregg-miskelly commented 1 year ago
        }

This is still active.

If it makes your life easier, you can allow a set method, but in this case I would make these other changes:


In reply to: 1404353242


Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad92d55785042d3851a4d472b8fbf985902, deletion_comment = False)

gregg-miskelly commented 1 year ago
    /// Don't change this test without checking with C++ team.

We are changing this test.


Refers to: src/MICoreUnitTests/BasicLaunchOptionsTests.cs:255 in 3cb7931. [](commit_id = 3cb7931ff5764cf126edadcdc14d80876a287471, deletion_comment = False)

gc46 commented 1 year ago
        }

This is still active.

If it makes your life easier, you can allow a set method, but in this case I would make these other changes:

  • Still initialize the backing field to an empty collection so you don't need those null checks.
  • Only allow set if the backing field is still an empty collection

In reply to: 1404353242

Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

The reason I allowed for set is due to the VisualizerFiles assignment in other places throughout the codebase such as the one shown below: image

We can disallow the ability to set VisualizerFiles though if you deem it fine.

WardenGnaw commented 1 year ago
        }

This is still active. If it makes your life easier, you can allow a set method, but in this case I would make these other changes:

  • Still initialize the backing field to an empty collection so you don't need those null checks.
  • Only allow set if the backing field is still an empty collection

In reply to: 1404353242 Refers to: src/MICore/LaunchOptions.cs:974 in 50400ad. [](commit_id = 50400ad, deletion_comment = False)

The reason I allowed for set is due to the VisualizerFiles assignment in other places throughout the codebase such as the one shown below: image

We can disallow the ability to set VisualizerFiles though if you deem it fine.

You can let LaunchOptions.VisualizerFiles be created during the constructor and it will be empty. When we need to assign/pass the Users strings you can use this.VisualizerFiles.AddRange(option.VisualizerFiles).