Friday, February 19, 2016

Some XAML basics

This is a quick overview of some of the fundamental XAML syntax rules for creating objects and setting properties. It's stuff that's so basic, most WPF guides won't even both to go over it. But if you don't understand these things, life as a WPF developer will be difficult for you.

I had to learn all this through trial and error and inductive reasoning; hopefully spelling it out here will save someone some else the blood, sweat, tears involved.

XAML is fundamentally about describing an object tree that you want the program to construct. You can order an object of a particular type to be created by writing an XML element with a name that matches the class name. For example:

<!-- Creates a default-constructed TextBlock -->
<TextBlock />

This commands the parser to create a TextBlock object using the class's default constructor. This syntax is not limited to WPF classes; you can create anything you like this way:

<!-- Creates a default-constructed String -->
<!-- Need to have 'xmlns:system="clr-namespace:System;assembly=mscorlib"' in document header -->
<system:String />

That's great, but default construction will only get you so far. How can we specify property values for the objects we create? The full, verbose way to do this is by using what is called element syntax. This is how you would set the Text property on a TextBlock using element syntax:

<TextBlock>
   <TextBlock.Text>Hello world</TextBlock.Text>
</TextBlock>

A few things to note here:
  1. Properties are initialized using sub-elements of the class element (property elements)
  2. Property element names must be given as ClassName.PropertyName
  3. The content of the property element gives the property value
This is all well and good, but it's also a lot of typing. That's why attribute syntax was invented. Here is an equivalent way to construct this TextBlock:

<TextBlock Text="Hello world" />

Attribute syntax saves a lot of electronic trees, but it only works when the value of the property in question can be parsed from a string. It can't help you in cases like this:

<ListView>
   <ListView.ItemTemplate>
      <DataTemplate />
   </ListView.ItemTemplate>
</ListView>

Here, the type of the ListView.ItemTemplate property is DataTemplate, and there is no way to construct a DataTemplate from a string.

This raises an interesting question: what types of objects can the XAML parser construct from strings? Strings, obviously, are trivially constructable from strings. Primitive numerics like Int32 and Double aren't much harder. But then you encounter something like this:

<TextBlock Margin="2,4,8,2" />

FrameworkElement.Margin is of type Thickness. Thickness is a struct that exposes four public properties: Left, Top, Right, and Bottom. How can the XAML parser construct this type from the string given? Does it have hard-coded knowledge of Thickness? Or maybe there's a rule that you can assign to properties in order using a comma-delimited list? It's actually neither. If you look at the source code for the Thickness class, you'll see this:

[TypeConverter(typeof (ThicknessConverter))]
public struct Thickness : IEquatable<Thickness>

The XAML parser will notice that TypeConverterAttribute, instantiate the type of converter it indicates (ThicknessConverter), and ask it to try to convert the String "2,4,8,2" into a Thickness. And ThicknessConverter knows how to do it. If you want to be able to instantiate your own types from strings in XAML, all you need to do is write an appropriate TypeConverter and attach a TypeConverterAttribute that points to it.

There's yet another technique for building objects from strings: the markup extension. The XAML for a markup extension looks like this:

<TextBlock Text="{StaticResource ApplicationName}" />

The XAML always begins with '{' and ends with '}'. The first word following the '{' is the name of a class that descends from MarkupExtension (if the class name ends with "Extension", as convention dictates, the word "Extension" can be omitted). After the class name is a space followed by a comma-delimited list of constructor arguments (if any). After the constructor arguments are optional property assignments as a comma-delimited list of "Name=Value" pairs.

When the XAML parser encounters a markup extension, it looks up and instantiates the type using any provided constructor arguments, initializes its properties as indicated, and then calls its virtual MarkupExtension.ProvideValue() method. This method returns an object, which is then assigned to the property on the left-hand side of the expression.

Markup extensions can also be nested inside of other markup extensions. For example:

<Grid IsHitTestVisible="{Binding IsOpen,
   RelativeSource={RelativeSource TemplatedParent},
   Converter={StaticResource InverseBoolConverter}}" />

Let's break down what's happening here:
  1. A Binding markup extension is instantiated, passing the string "IsOpen" to its constructor
  2. A RelativeSource markup extension is instantiated, passing the enum value RelativeSourceMode.TemplatedParent to its constructor
  3. RelativeSource.ProvideValue() is called, and the return value is assigned to the Binding.RelativeSource property
  4. A StaticResourceExtension markup extension is instantiated, passing the string "InverseBoolConverter" to its constructor
  5. StaticResourceExtension.ProvideValue() is called, and the return value is assigned to the Binding.Converter property
  6. Binding.ProvideValue() is called, and the return value is assigned to the Grid.IsHitTestVisible property
A lot going on, but it's all pretty standard and comprehensible once you understand what the syntax means.

Now that you know about markup extensions, I can make something I said earlier less untrue (this is a technique called teaching by diminishing deception). Consider this code again:

<ListView>
   <ListView.ItemTemplate>
      <DataTemplate />
   </ListView.ItemTemplate>
</ListView>

I said before that ListView.ItemTemplate needs to be set using element syntax because there's no way to construct a DataTemplate from a string. You now know this isn't trueyou could use a markup extension. An obvious candidate is StaticResourceExtension:

<UserControl>
   <UserControl.Resources>
      <DataTemplate x:Key="MyDataTemplate" />
   </UserControl.Resources>

   <ListView ItemTemplate="{StaticResource MyDataTemplate}" />
</UserControl>

Now you know another rule of XAMLthere are always about a half dozen ways to achieve any particular goal.

Wednesday, January 27, 2016

Copy an SVG image to the clipboard from C#

Despite the title, this is really an article about putting arbitrary binary data onto the clipboard from .net.

Suppose you have the XML text corresponding to an SVG image stored in a string, and you'd like to put in on the clipboard as an image (not as text). The MIME type is "image/svg+xml", so it seems like the solution is straightforward:

string svg = GetSvg();
Clipboard.SetData("image/svg+xml", svg); // won't work

However, if you do this and then inspect the contents of the clipboard, you'll see that .net added a number of bytes in front of your xml text:


That's no good, and it means any SVG editor is going to reject an attempt to paste the image. Maybe we should try straight binary data instead of a string?

string svg = GetSvg();
byte[] bytes = Encoding.UTF8.GetBytes(svg);
Clipboard.SetData("image/svg+xml", bytes); // still won't work

This gives the same result, but with a slightly different set of extra bytes prepended. The fact that the prefix changed slightly when the datatype changed suggests we might be looking at a serialization header. And indeed, if you dig through the MSDN documentation, you might eventually find this note with the Clipboard class (emphasis mine):
An object must be serializable for it to be put on the Clipboard. If you pass a non-serializable object to a Clipboard method, the method will fail without throwing an exception. See System.Runtime.Serialization for more information on serialization. If your target application requires a very specific data format, the headers added to the data in the serialization process may prevent the application from recognizing your data. To preserve your data format, add your data as a Byte array to a MemoryStream and pass the MemoryStream to the SetData method.
Aha! Take 3:

string svg = GetSvg();
byte[] bytes = Encoding.UTF8.GetBytes(svg);
MemorySteam stream = new MemoryStream(bytes);
Clipboard.SetData("image/svg+xml", stream);

This, finally, will give the result you want. The same technique will work with DataObject, in case you want to copy a bitmap version of the image alongside the SVG.

Might have been nice to have that little note with Clipboard.SetData() too, but such is the life of a software developer. :)

Monday, July 20, 2015

WPF: Animate the removal of a ListViewItem from a ListView


Animating the addition of a new item to a ListView is relatively straightforward. See this question and answer on StackOverflow, for example. But doing the same thing when an item is removed is more difficult. This is because, by the time events like Unloaded go out, the control has already been removed from the tree.

The key to making this work is to add a "MarkedForDeletion" concept somewhere. The act of marking an item for deletion should trigger the animation, and the completion of the animation should trigger the actual removal from the underlying collection. I successfully implemented this idea using an attached behavior I called AnimateItemRemovalBehavior. This class exposes three public properties, all of which must be set on each ListViewItem:
  1. Storyboard of type Storyboard. This is the actual animation you want to run when an item is removed.
  2. PerformRemoval of type ICommand. This is a command that will be executed when the animation is done running. It should execute code to actually remove the element from the databound collection.
  3. IsMarkedForRemoval of type bool. Set this to true when you decide to remove an item from the list (e.g. in a button click handler). As soon as the attached behavior sees this property change to true, it will begin the animation. And when the animation is complete, the PerformRemoval command will Execute.
With that said, let's look at the code! The behavior itself looks like this:

namespace Demo
{
    using System;
    using System.Windows;
    using System.Windows.Input;
    using System.Windows.Media.Animation;

    internal class AnimateItemRemovalBehavior
    {
        public static readonly DependencyProperty StoryboardProperty =
            DependencyProperty.RegisterAttached(
                "Storyboard",
                typeof(Storyboard),
                typeof(AnimateItemRemovalBehavior),
                null);

        public static Storyboard GetStoryboard(DependencyObject o)
        {
            return o.GetValue(StoryboardProperty) as Storyboard;
        }

        public static void SetStoryboard(DependencyObject o, Storyboard value)
        {
            o.SetValue(StoryboardProperty, value);
        }



        public static readonly DependencyProperty PerformRemovalProperty =
            DependencyProperty.RegisterAttached(
                "PerformRemoval",
                typeof(ICommand),
                typeof(AnimateItemRemovalBehavior),
                null);

        public static ICommand GetPerformRemoval(DependencyObject o)
        {
            return o.GetValue(PerformRemovalProperty) as ICommand;
        }

        public static void SetPerformRemoval(DependencyObject o, ICommand value)
        {
            o.SetValue(PerformRemovalProperty, value);
        }



        public static readonly DependencyProperty IsMarkedForRemovalProperty =
            DependencyProperty.RegisterAttached(
                "IsMarkedForRemoval",
                typeof(bool),
                typeof(AnimateItemRemovalBehavior),
                new UIPropertyMetadata(OnMarkedForRemovalChanged));

        public static bool GetIsMarkedForRemoval(DependencyObject o)
        {
            return o.GetValue(IsMarkedForRemovalProperty) as bool? ?? false;
        }

        public static void SetIsMarkedForRemoval(DependencyObject o, bool value)
        {
            o.SetValue(IsMarkedForRemovalProperty, value);
        }



        private static void OnMarkedForRemovalChanged(DependencyObject d,
            DependencyPropertyChangedEventArgs e)
        {
            if ((e.NewValue as bool?) != true)
                return;

            var element = d as FrameworkElement;
            if (element == null)
                throw new InvalidOperationException(
                    "MarkedForRemoval can only be set on a FrameworkElement");

            var performRemoval = GetPerformRemoval(d);
            if (performRemoval == null)
                throw new InvalidOperationException(
                    "MarkedForRemoval requires PerformRemoval to be set too");

            var sb = GetStoryboard(d);
            if (sb == null)
                throw new InvalidOperationException(
                    "MarkedForRemoval requires Stoyboard to be set too");

            if (sb.IsSealed || sb.IsFrozen)
                sb = sb.Clone();

            Storyboard.SetTarget(sb, d);
            sb.Completed += (_, __) =>
            {
                var vm = element.DataContext;
                if (!performRemoval.CanExecute(vm))
                    return;
                performRemoval.Execute(vm);
            };

            sb.Begin();
        }
    }
}

And here is some XAML giving example usage:

<Window x:Class="Demo.MainWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    xmlns:local="clr-namespace:Demo"
    mc:Ignorable="d"
    Title="MainWindow" Height="350" Width="525">

    <Window.Resources>
        <local:PersonListViewModel x:Key="ViewModel" />
    </Window.Resources>
    
    <DockPanel>
        <TextBlock DockPanel.Dock="Top" Margin="0,0,0,10"
            Text="Click an item to remove it from the list" />
        
        <ListView DataContext="{StaticResource ViewModel}"
            ItemsSource="{Binding PersonList}">
            
            <ListView.ItemContainerStyle>
                <Style TargetType="ListViewItem">
                    <Setter Property="local:AnimateItemRemovalBehavior.Storyboard">
                        <Setter.Value>
                            <Storyboard>
                                <DoubleAnimation Storyboard.TargetProperty="Opacity"
                                    From="1.0" To="0.0" Duration="0:0:0.2" />
                            </Storyboard>
                        </Setter.Value>
                    </Setter>

                    <Setter Property="local:AnimateItemRemovalBehavior.IsMarkedForRemoval"
                        Value="{Binding IsMarkedForRemoval}" />

                    <Setter Property="local:AnimateItemRemovalBehavior.PerformRemoval"
                        Value="{Binding RelativeSource={RelativeSource FindAncestor,
                            AncestorType={x:Type ListView}},
                            Path=DataContext.RemovePersonCommand}" />
                </Style>
            </ListView.ItemContainerStyle>

            <ListView.ItemTemplate>
                <DataTemplate>
                    <TextBlock Text="{Binding Name}">
                        <TextBlock.InputBindings>
                            <MouseBinding MouseAction="LeftClick"
                                Command="{Binding MarkForRemovalCommand}" />
                        </TextBlock.InputBindings>
                    </TextBlock>
                </DataTemplate>
            </ListView.ItemTemplate>
        </ListView>
    </DockPanel>
</Window>

And finally, some example viewmodel code to go with the XAML:

namespace Demo
{
    using System;
    using System.Collections.ObjectModel;
    using System.ComponentModel;
    using System.Runtime.CompilerServices;
    using System.Windows;
    using System.Windows.Input;

    class ViewModelBase : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        protected virtual void OnPropertyChanged(
            [CallerMemberName] string propertyName = null)
        {
            PropertyChanged?.Invoke(this,
                new PropertyChangedEventArgs(propertyName));
        }
    }

    public class RelayCommand<T> : ICommand where T : class
    {
        private readonly Action<T> _methodToExecute;
        private readonly Func<T, bool> _canExecuteEvaluator;

        public event EventHandler CanExecuteChanged
        {
            add { CommandManager.RequerySuggested += value; }
            remove { CommandManager.RequerySuggested -= value; }
        }

        public RelayCommand(Action<T> methodToExecute,
            Func<T, bool> canExecuteEvaluator = null)
        {
            _methodToExecute = methodToExecute;
            _canExecuteEvaluator = canExecuteEvaluator;
        }

        public bool CanExecute(object parameter)
        {
            return _canExecuteEvaluator?.Invoke(parameter as T) ?? true;
        }

        public void Execute(object parameter)
        {
            _methodToExecute.Invoke(parameter as T);
        }
    }



    class PersonViewModel : ViewModelBase
    {
        public PersonViewModel(string name)
        {
            Name = name;
            MarkForRemovalCommand = new RelayCommand<object>(MarkForRemoval);
        }

        public string Name { get; }

        private bool _isMarkedForRemoval;
        public bool IsMarkedForRemoval
        {
            get { return _isMarkedForRemoval; }

            set
            {
                if (_isMarkedForRemoval == value)
                    return;
                _isMarkedForRemoval = value;
                OnPropertyChanged();
            }
        }

        public ICommand MarkForRemovalCommand { get; }
        private void MarkForRemoval(object argument)
        {
            IsMarkedForRemoval = true;
        }
    }



    class PersonListViewModel : ViewModelBase
    {
        public PersonListViewModel()
        {
            PersonList = new ObservableCollection<PersonViewModel>
            {
                new PersonViewModel("Fred"),
                new PersonViewModel("Barney"),
                new PersonViewModel("Wilma"),
                new PersonViewModel("Pebbles"),
                new PersonViewModel("Bam-bam")
            };

            RemovePersonCommand = new RelayCommand<PersonViewModel>(RemovePerson);
        }

        public ObservableCollection<PersonViewModel> PersonList { get; }

        public ICommand RemovePersonCommand { get; }
        private void RemovePerson(PersonViewModel person)
        {
            PersonList.Remove(person);
        }
    }



    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();
        }
    }
}

Saturday, July 18, 2015

WPF: Using an object as the Value in a Style Setter will seal and freeze it

The title says it all. I discovered this during an attempt to animate the addition of a ListViewItem to a ListView. I decided to do this via an attached behavior that involved an attached property of type Storyboard. The XAML looked something like this:

<Window>
   <Window.Resources>
      <Storyboard x:Key="TheAnimation" x:Shared="False">
         <DoubleAnimation From="0.0" To="1.0" Duration="0:0:0.20"
            Storyboard.TargetProperty="Opacity" />
      </Storyboard>
   </Window.Resources>
    
   <Grid>
      <ListView>
         <ListView.Resources>
            <Style TargetType="{x:Type ListViewItem}">
               <Setter Property="local:MyBehavior.Storyboard"
                  Value="{StaticResource TheAnimation}" />
            </Style>
         </ListView.Resources>
      </ListView>
   </Grid>
</Window>

As you can see, because the ListViewItems are not directly accessibly in XAML, I used a Style to set their MyBehavior.Storyboard attached properties. Then, in the C# code for the behavior, I tried to do this:

private static void StoryboardChanged(DependencyObject d,
   DependencyPropertyChangedEventArgs e)
{
   var listViewItem = d as ListViewItem;
   if (d == null)
      return;

   var sb = e.NewValue as Storyboard;
   if (sb == null)
      return;

   Storyboard.SetTarget(sb, listViewItem);
   sb.Begin();
}

But this blew up with an InvalidOperationException on the call to Storyboard.SetTarget():
Cannot set a property on object 'System.Windows.Media.Animation.Storyboard' because it is in a read-only state
And indeed, inspecting sb in the debugger showed that its IsSealed and IsFrozen properties were both set to true.

Searching around the web turned up evidence that Styles are indeed sealed by design the first time they're used. For example, this page at MSDN ("Note that once a style has been applied, it is sealed and cannot be changed. If you want to dynamically change a style that has already been applied, you must create a new style to replace the existing one"). Examining the source code confirms this, and also demonstrates that sealing a Style seals its Setters, and sealing a Setter seals its Values. So there's no mystery about what happened, but it's not at all clear why all of this sealing is being done in the first place. There are a few claims that this has to do with thread safety, for example, here and here. That seems plausible, but I wonder if there isn't another reason toothe Style node in the XAML presumably translates into a single Style object that is shared among all the objects that consume it. If one of them were to modify it (or its contents) in some way, the change would affect everyone, which is probably not what you want. This is just a guess; I haven't been able to prove it, but it makes sense. Assuming, of course, that I am correct in presuming that only one Style object is created.

In any case, in this case, I was able to solve my problem simply by cloning the Storyboard and working with the clone:

if(sb.IsSealed || sb.IsFrozen)
   sb = sb.Clone();

A more general solution might be to make the property type XyzFactory instead of Xyz, though that would obviously limit your ability to put things together in markup.

Friday, May 8, 2015

The shared_ptr aliasing constructor

The shared_ptr aliasing constructor is one of the more obscure corners of C++11. It allows you to create a shared_ptr that points to one object, but owns another. The pointed to object is what you get from operator*, operator->, etc.; the owned object is the one that is deleted when the reference count reaches zero.

Why would you want to separate these two things?

Use case 1


The motivating case was a shared_ptr that owns an instance of some complex class, but points to one of its sub-objects. For example:

struct Bar {};

struct Foo
{
   Bar bar;
};

class OwnsBar
{
private:
   const std::shared_ptr<Bar> m_bar;

public:
   OwnsBar(const std::shared_ptr<Bar>& bar)
      : m_bar(bar)
   {
   }
};

std::shared_ptr<OwnsBar> CreateBarOwner()
{
   auto foo = std::make_shared<Foo>();
   std::shared_ptr<Bar> aliasBarPtr(foo, &foo->bar);
   return std::make_shared<OwnsBar>(aliasBarPtr);
}

Here, aliasBarPtr owns the entire Foo struct and keeps it alive, but it points to the Bar member. This means that (a) aliasBarPtr can be used anywhere a shared_ptr<Bar> is expected, and (b) aliasBarPtr will keep the entire parent Foo alive even after the local foo variable goes out of scope.

Use case 2


Another use for the aliasing constructor is when some (badly-designed, most likely) function wants a std::shared_ptr<Bar>, but the Bar you have isn't in a shared_ptr:

void f(const std::shared_ptr<Bar>& bar);

void g(Bar& bar)
{
   // don't actually do this unless you have no choice; see comments below
   std::shared_ptr<Bar> fakeSharedPtrBar(std::shared_ptr<Bar>(), &bar);
   f(fakeSharedPtrBar);
}

fakeSharedPtrBar owns nothing, but it points to bar. In effect, it is a raw pointer masquerading as a shared_ptr.

Note that pulling this little sleight of hand is dangerous; if f passes the fake shared_ptr to someone else who hangs onto it beyond the point that g's Bar is destroyed, things will blow up. Only use this hack if you're absolutely sure f won't do this and refactoring it to take a raw pointer or reference isn't an option.

Use case 3


One more use for the aliasing constructor is to tie object lifetimes together. For example, suppose you have a shared_ptr<Document> and two ambient shared_ptr<IDocumentObservers> that you want to keep alive for exactly as long as the Document stays around. The aliasing constructor can help:

// Utility function to relocate an object into a shared_ptr using its move constructor
// obj will no longer be valid upon returning
template<typename T>
std::shared_ptr<T> MoveIntoSharedPtr(T& obj)
{
   return std::make_shared<T>(std::move(obj));
}

std::shared_ptr<Document> CreateDocument()
{
   auto doc = std::make_shared<Document>();

   auto observer1 = std::make_shared<Observer1>();
   doc->AddWeakObserver(std::weak_ptr<IDocumentObserver>(observer1));

   auto observer2 = std::make_shared<Observer2>();
   doc->AddWeakObserver(std::weak_ptr<IDocumentObserver>(observer2));

   auto keepAliveTuple = std::make_tuple(doc, observer1, observer2);

   return std::shared_ptr<Document>(MoveIntoSharedPtr(keepAliveTuple), doc.get());
}

In a previous post, I used a custom deleter to achieve this same thing. That works almost all the time, but you can get odd bugs if the type in question (Document, in the example above) implements enable_shared_from_this. See this stackoverflow question for details.

Tuesday, March 17, 2015

Don't use an MFC CArray with types that contain self pointers or back pointers

CArray is MFC's resizing array class. Its semantics are similar to std::vector's, and in new code you should prefer vector because of the standard library support that comes with it (and because of the gotcha I'm about to describe). But if you're working with an MFC legacy codebase or interacting with the MFC framework itself, you will at times find yourself needing to use CArray. And CArray can get you into trouble if you use it with the wrong types.

The difficulty comes from the way CArray handles the copy operation for its elements when its buffer is relocated in memory as part of a resize operation. vector and the other standard library containers copy their elements one at a time using the contained type's copy/move constructor, but CArray just does a single bulk memcpy from the old buffer space into the new one. If your type is POD this is fine, but if it defines a copy constructor, it won't be called and things will probably blow up (and Microsoft does document this on their CArray page). In particular, if there are any sort of self pointers or back pointers involved, they will become wild. For example, consider this code:

void TraceAddress(const CString& prefix, void* p)
{
 CString str;
 str.Format(_T("%s: %p\n"), prefix.GetString(), p);
 TRACE(str);
}

void Demo()
{
 struct Foo
 {
  Foo() : me(this) {}
  Foo(const Foo&) : me(this) {}
  Foo* me;
 };

 TRACE(_T("\nCArray:\n"));
 CArray<Foo> aFoo;
 
 aFoo.SetSize(1);
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);
 
 aFoo.SetSize(20);
 TRACE(_T("Array resized\n"));
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);

 TRACE(_T("\nVector:\n"));
 std::vector<Foo> vFoo;
 
 vFoo.resize(1);
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
 
 vFoo.resize(20);
 TRACE(_T("Vector resized\n"));
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
}

The output I saw when I ran this on my machine was:

CArray:
Address of aFoo[0]: 0063AC90
me ptr  of aFoo[0]: 0063AC90
Array resized
Address of aFoo[0]: 0063ACD0
me ptr  of aFoo[0]: 0063AC90

Vector:
Address of vFoo[0]: 0063AC90
me ptr  of vFoo[0]: 0063AC90
Vector resized
Address of vFoo[0]: 0063ADA8
me ptr  of vFoo[0]: 0063ADA8

What happened here? When we called aFoo.SetSize(1), the CArray reserved a buffer large enough to hold a single Foo at address 0063AC90. It then constructed a Foo there using the class's default constructor, which correctly initializes the me pointer. Then, when we called aFoo.SetSize(20), it reserved a new buffer large enough to hold 20 Foos at address 006ACD0 and copied the existing Foo object to this location. But instead of using the copy constructor to do this, it just memcpyied the thing, which left the me member pointing to the old address. This is a bug, but you won't discover it until the next time someone tries to use that me pointer. By then, the thread could be miles away from the resize operation leaving you with little clue how things went bad. vector, by contrast, uses Foo's copy constructor when it needs to relocate its buffer, so everything works out ok.

This case may seem obvious and contrived, but sometimes the self references are indirect and less easy to detect. For example, a co-worker recently called me over to help him with a tough, reproducible bug he couldn't figure out. A function took a const vector<float>& argument and then performed a std::max_element operation on it, but when max_element went to construct an iterator for its return value, a checked iterator failure was occurring. He was convinced he'd found a bug in the standard library, but I wasn't so sure. Tracing back a ways, it turned out that the input vector was ultimately a member of a struct that was being stored in a CArray. Something like this:

struct S
{
   std::vector<float> v;
};

CArray<S> sArray;
//...
FunctionThatFailed(sArray[0].v);



Upon seeing this, I immediately suspected that he'd stumbled into a CArray resize bug. And sure enough, it turns out that under our standard library implementation (MSVC++ 2012), vector contains a pointer to a proxy (_Myproxy) which in turn contains a pointer back to the container it proxies for (_Mycont). When that CArray got resized it broke all those _Mycont back pointers for the existing elements, but this didn't show up until much later when iterator creation was triggered by calling std::max_element on one of the individual vectors. I suggested he replace the CArray with a vector, and once he did, everything worked fine.

Monday, February 23, 2015

Dependency injection with interface-based programming: how to avoid repeating constructor arguments ad nauseum

I've already written about interface-based programming in C++ here. I haven't written specifically about dependency injection, but there's no shortage of material out there on that subject; for example here, here, and here. It's natural to want to combine the two, but in C++ at least, doing so has the unfortunate side effect of forcing you to repeat the dependency list many times. For example, consider a class with three dependencies: Foo, Bar, and Baz (for the sake of simplicity, suppose they will all be passed and stored in shared_ptrs). First, we need to list these in the declaration of the class's public factory method:

// MyClass.h
#include <memory>

class MyClass
{
public:
   static std::unique_ptr<MyClass> Create(
      const std::shared_ptr<Foo>& foo,
      const std::shared_ptr<Bar>& bar,
      const std::shared_ptr<Baz>& baz);
   // ...
};

Then again as member variables in the impl class:

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
   private:
      const std::shared_ptr<Foo> m_foo;
      const std::shared_ptr<Bar> m_bar;
      const std::shared_ptr<Baz> m_baz;
      // ...
   };
}

Then a third time as constructor arguments to the impl class, and a fourth and fifth in the constructor's initializer list (thankfully, we only need the names here, not the types):

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
      // ...
   public:
      MyClassImpl(
         const std::shared_ptr<Foo>& foo,
         const std::shared_ptr<Bar>& bar,
         const std::shared_ptr<Baz>& baz)
         : m_foo(foo)
         , m_bar(bar)
         , m_baz(baz)
      {
      }
      // ...
   };
}

A sixth statement is needed in the signature of the factory method's implementation, and finally, a seventh in that method's body when we instantiate the impl (again, with just the names this time):

// MyClass.cpp
// ...
std::unique_ptr<MyClass> MyClass::Create(
      const std::shared_ptr<Foo>& foo,
      const std::shared_ptr<Bar>& bar,
      const std::shared_ptr<Baz>& baz)
{
   return std::unique_ptr<MyClass>(new MyClassImpl(foo, bar, baz));
   // use std::make_unique with C++14
}

That's a lot of repetition. It means if you want to add or remove or just rename a dependency, you need to edit code in a minimum of seven different places.

A simple thing we can do to reduce the pain is to wrap all the dependencies up in a single struct:

// MyClass.h
#include <memory>

class MyClass
{
public:
   struct Dependencies
   {
      std::shared_ptr<Foo> foo;
      std::shared_ptr<Bar> bar;
      std::shared_ptr<Baz> baz;
   };

   static std::unique_ptr<MyClass> Create(const Dependencies& dependencies);
   // ...
};

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
   private:
      const Dependencies m_d;

   public:
      MyClassImpl(const Dependencies& d)
         : m_d(d)
      {
      }
      // ...
   };
}

std::unique_ptr<MyClass> MyClass::Create(const Dependencies& d)
{
   return std::unique_ptr<MyClass>(new MyClassImpl(d));
   // use std::make_unique with C++14
}

We still have to repeat Dependencies itself the same number of times, but it's only one thing, and more importantly, it stays the same even if we modify the dependency list. Further, it's really easy to make reusable code snippets out of something like the above (minus the actual contents of the Dependencies struct).

However, there's another problem now. When each dependency was a constructor argument, it was impossible to forget one. That isn't the case when they are struct members; for example, with the original code, the compiler would reject something like this:

auto myObject = MyClass::Create(someFoo, someBar); // oops, forgot the Baz

But with the new code, the below would build with no problems:

MyClass::Dependencies d;
d.foo = someFoo;
d.bar = someBar;
// oops, forgot the Baz
auto myObject = MyClass::Create(d);

We could handle this by validating that d.baz != nullptr inside MyClassImpl's constructor, but that wouldn't happen until runtime. Your first thought might be to give Dependencies a constructor instead of using memberwise initialization, but then we're back to the original problem of having to repeat all the arguments over and over (though admittedly not as many times).

Another idea is to use aggregate initialization:

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   someBaz
};
auto myObject = MyClass::Create(d);

This looks promising, but what happens if we forget that Baz now?

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   // oops; forgot the baz
};
auto myObject = MyClass::Create(d);

Unfortunately for our current purposes, this still compiles; d.baz will simply be initialized using shared_ptr's default constructor. But there is a way to prevent thiswe just need to make sure the last member in Dependencies is something that cannot be default-constructed. For example:

// FinalMember.h
struct FinalMember
{
   FinalMember(int) {}
};

// MyClass.h
#include "FinalMember.h"

class MyClass
{
public:
   struct Dependencies
   {
       const std::shared_ptr<Foo> foo;
       const std::shared_ptr<Bar> bar;
       const std::shared_ptr<Baz> baz;
       FinalMember finalMember;
   };
   // ...
};

Now callers have no choice but to initialize all the members:

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   someBaz,
   FinalMember(0) // can't omit or put in wrong position without a compile error
};
auto myObject = MyClass::Create(d);

Note that FinalMember could be replaced with anything that can't be value-initialized, including a plain const & to anything, but using a named type helps communicate our intent more clearly.

Downside (maybe): You now must use aggregate initialization when you set up new instances of Dependencies. That's not so bad (and in fact it allows us to make every member const, which makes me feel all warm and fuzzy), but since aggregate initialization is not used all that often with structs, your IDE may not understand what you're doing well enough to help you put things in the right order. Also, some compilers may be uncomfortable with the fact that Dependencies has no user-defined constructors since a default one cannot be generated. For example, with Visual Studio 2012, the code above triggers 3 warnings (the last one is simply incorrect, as we've seen):

warning C4510: 'MyClass::Dependencies' : default constructor could not be generated
warning C4512: 'MyClass::Dependencies' : assignment operator could not be generated
warning C4610: struct 'MyClass::Dependencies' can never be instantiated - user defined constructor required

I have to go to the trouble of disabling these with a pragma since I build with "treat warnings as errors" enabled. But if you (and your tools) can get past this, there's a lot to be said for the convenience this approach can offer.