Title: Are we nearly there yet? Refactoring C++

Author: Martin Moene

Date: 11 July 2015 21:42:29 +01:00 or Sat, 11 July 2015 21:42:29 +01:00

Summary: Alan Griffiths evaluates two tools for developers with some simple use-cases.

Body: 

Refactoring

The idea of invariance under transformation is ancient and the fundamental concept behind the mathematical representation of symmetry. It has applications in many disciplines: physics employs the symmetries of eleven dimensions for ‘string theory’; music employs various temporal and tonal symmetries; even politics references the symmetry of ‘right’ and ‘left’.

In software development there are various symmetries, but one important class of transformations are the refactorings of code. I’ve used these over many years but hadn’t recognised their significance until I encountered Martin Fowler’s book of that name [1] around the turn of the millennium.

Shortly after that I was working in Java and encountered the first attempts at providing automation of refactoring transformations of code in development tools: first in Eclipse and then in JetBrain’s IntelliJ IDEA. The experience was a revelation to me about how I’d been avoiding making transformations to the sourcecode simply because it was too much effort to keep track of things.

To take a simple example: extracting a piece of a function into a new, more focussed, function. I’m sure you’ve been there too: a function starts out with a clear purpose but gradually grows over time until the first part of it does one thing and the rest something that follows. Then some code doesn’t need the first part done that way and a conditional branch is added.

If we were starting from scratch, the two parts belong in separate functions but it requires work to split them out: one needs to work out the parameters that need passing, track the scope of local variables, create function prototypes, and move the code. None of that is hard, but the ‘housekeeping’ takes headspace and the effort can interrupt the more urgent task at hand.

The important thing to know about refactorings is the invariant: they don’t change the meaning of code. It behaves exactly the same before and after the refactoring – only the design changes. I mentioned the ‘Extract Method’ refactoring above – there’s a corresponding refactoring ‘Inline Method’ that replaces each call of a method with the method body. Refactoring isn’t a goal in itself: you use a refactoring to reliably change the code in a way that makes it more useful for something the code doesn’t currently support.

What refactoring tools gave me when working in Java (and later in C# and Python) was the ability to select the transformation I needed and let the editor/IDE to do the job. When you can do that with the same assurance that you can use automated indentation or ‘auto complete’ you don’t have to worry about breaking the code or interrupt your thinking about the code.

But what about C++?

With C++ being the lingua franca of ACCU I shouldn’t need mention that C++ is hard – not just for developers, but also for the writers of development tools. There’s a reason that C++ compilers are orders of magnitude slower than Java compilers at processing lines of sourcecode: the language is more complex. (I don’t think anyone has even dreamt of implementing Eratosthenes Sieve at compile time in Java.) And the preprocessor provides lots of opportunity for confusing tools.

So, although over a decade has passed since I used refactoring tools in Java, whenever I start a C++ job I have to revert to making these transformations the hard way.

Eclipse CDT and Cevelop

Of course, C++ hasn’t been ignored completely for a decade and a half and work (notably that led by a well known ACCUer: Peter Somerland) has continued on implementing C++ support as Eclipse plugins. CDT [2] is the original project and Cevelop [3] is based on CDT as packaged by Peter’s group at the Institute for Software.

I’ll give some examples below, but while the support this gives is better than nothing it isn’t the reliable refactoring support needed to avoid breaking the flow. In practice I find that I need to commit the state of the code before any refactoring to avoid losing work and build and run the test suite afterwards. Some very strange things can happen and, if problems occur during the refactoring, the editor can revert the code to an old (or even broken) state losing recent changes.

In spite of these problems I have made a lot of use of Eclipse CDT and Cevelop over the last few years. (Not that it has entirely separated me from vim – I use different tools for different tasks.)

JetBrains CLion

I mentioned JetBrains above: they are the company behind some popular refactoring IDEs and plugins for Java, C# and Python. Last year they announced a beta program for their C++ refactoring tool – and based on my past good experience with their tools I signed up. However, I didn’t use their IDE much during the beta program mostly because it was very slow – at one point taking tens of seconds to respond to each key-press.

But CLion was recently released and I downloaded the evaluation version. I still had some problems getting it to work but as they were exhibiting at the ACCU conference and took an interest in the problems I was seeing: I got to a point where I could evaluate it. There are still some performance problems (mostly a 5 minute start-up time on my current project) but it proved usable.

As with Cevelop the automated refactoring can go badly wrong (again I’ll give some examples), but so far I’ve always been able to fix or just revert the refactoring without problems.

I’m going to concentrate on Cevelop in what follows, but a lot of what I say about it will also apply to Eclipse-CDT.

My current project: Mir

I’m currently working on an open-source C++ project – which means that you can download the code [4] and experiment for yourself. It is part of Canonical’s ‘Ubuntu Touch’ phone operating system and intended to form part of a future ‘converged’ desktop and phone version of Linux.

The Mir code is mostly C++11 with some of the more recent code using bits of C++14. There are also a few C99 files around as we provide a C API. It isn’t an enormous project, but larger than many: about 250KLOC.

Starting off

There’s an immediate difference between the startup of CLion and Cevelop as CLion recognises the project’s build system (CMake) and sets up an out-of-tree build environment. That’s nice, but it is a happy coincidence for me: if I needed to work on a project using an unsupported build system then I’d have to look elsewhere.

Cevelop in contrast gives the user the freedom to specify the build commands and the responsibility to set up the build environment.

Both then start scanning the codebase for symbols and until that finishes a lot of features are unavailable. There is a difference if the IDE is closed down and reopened as Cevelop appears to cache its ‘index’ whereas there is the same delay every time CLion is started.

Refactoring

I tried a bit of ‘Extract Method’ in one of the messier functions in the codebase and both IDEs failed horribly.

Listing 1 shows it in Cevelop. For reasons that are not obvious to me the new function isn’t a member function. (Even though it references the focus_controller member variable!)

$ bzr diff
=== modified file 'playground/demo-shell/window_manager.cpp'
--- playground/demo-shell/window_manager.cpp 2015-05-13 07:23:52 +0000
+++ playground/demo-shell/window_manager.cpp 2015-05-17 14:55:49 +0000
@@ -196,6 +196,12 @@
   surf.resize({right-left, bottom-top});
 }
 
+void select_next_session() {
+  focus_controller->focus_next_session();
+  if (const auto surface = focus_controller->focused_surface())
+    focus_controller->raise( { surface });
+}
+
 bool me::WindowManager::handle_key_event(MirKeyboardEvent const* kev)
 {
   // TODO: Fix android configuration and remove static hack ~racarr
@@ -210,9 +216,7 @@
  if (modifiers & mir_input_event_modifier_alt &&
    scan_code == KEY_TAB)  // TODO: Use keycode once we support
                           // keymapping on the server side
  {
-    focus_controller->focus_next_session();
-    if (auto const surface = focus_controller->focused_surface())
-        focus_controller->raise({surface});
+      select_next_session();
     return true;
   }
   else if (modifiers & mir_input_event_modifier_alt &&
			

Listing 2 shows the same in CLion. Not impressive either!

$ bzr diff                                                                                
=== modified file 'playground/demo-shell/window_manager.cpp'
--- playground/demo-shell/window_manager.cpp	2015-05-13 07:23:52 +0000
+++ playground/demo-shell/window_manager.cpp	2015-05-17 15:11:15 +0000
@@ -196,7 +196,11 @@
   surf.resize({right-left, bottom-top});
 }
 
-bool me::WindowManager::handle_key_event(MirKeyboardEvent const* kev)
+bool void WindowManager::select_next_session() const {
+  focus_controller->focus_next_session();
+  if (auto const surface = focus_controller->focused_surface())
+    focus_controller->raise({surface});
+} me::WindowManager::handle_key_event(MirKeyboardEvent const* kev)
 {
   // TODO: Fix android configuration and remove static hack ~racarr
   static bool display_off = false;
@@ -210,9 +214,7 @@
   if (modifiers & mir_input_event_modifier_alt &&
     scan_code == KEY_TAB)  // TODO: Use keycode once we support
                            // keymapping on the server side
   {
-    focus_controller->focus_next_session();
-    if (auto const surface = focus_controller->focused_surface())
-      focus_controller->raise({surface});
+    select_next_session();
     return true;
   }
   else if (modifiers & mir_input_event_modifier_alt &&
			

In neither case does the ‘refactored’ code even compile. In one sense that is good, as it flags up the problem; in another sense it is bad as for automated refactoring to be useful it is essential that it preserves the existing functionality. I tried a number of other examples with equally odd results.

I had better luck with ‘Extract Variable’, first CLion (Listing 3).

$ bzr diff
=== modified file 'examples/server_example_canonical_window_manager.cpp'
--- examples/server_example_canonical_window_manager.cpp 2015-05-14 12:29:13 +0000
+++ examples/server_example_canonical_window_manager.cpp 2015-05-17 15:51:29 +0000
@@ -113,7 +113,8 @@
 -> ms::SurfaceCreationParameters
 {
   auto parameters = request_parameters;
-  parameters.size.height = parameters.size.height + DeltaY{title_bar_height};
+  auto title_bar_delta = DeltaY{title_bar_height};
+  parameters.size.height = parameters.size.height + title_bar_delta;
 
   auto const active_display = tools->active_display();
 
@@ -198,8 +199,8 @@
         parameters.top_left.y = display_area.top_left.y;
   }
 
-  parameters.top_left.y = parameters.top_left.y + DeltaY{title_bar_height};
-  parameters.size.height = parameters.size.height - DeltaY{title_bar_height};
+  parameters.top_left.y = parameters.top_left.y + title_bar_delta;
+  parameters.size.height = parameters.size.height - title_bar_delta;
   return parameters;
 }
			

While I’d rather see auto const title_bar_delta that’s not bad – although I selected the second instance of DeltaY{ title_bar_height} it replaced all three correctly. Not so good with Cevelop (Listing 4).

$ bzr diff                                                                          
=== modified file 'examples/server_example_canonical_window_manager.cpp'
--- examples/server_example_canonical_window_manager.cpp 2015-05-14 12:29:13 +0000
+++ examples/server_example_canonical_window_manager.cpp 2015-05-17 16:01:41 +0000
@@ -197,8 +197,9 @@
       if (parameters.top_left.y < display_area.top_left.y)
         parameters.top_left.y = display_area.top_left.y;
   }
-
-  parameters.top_left.y = parameters.top_left.y + DeltaY{title_bar_height};
+  mir::geometry::DeltaY title_bar_delta = DeltaY
+  { title_bar_height };
+  parameters.top_left.y = parameters.top_left.y + title_bar_delta;
   parameters.size.height = parameters.size.height - DeltaY{title_bar_height};
   return parameters;
 }
			

This only replaced the selected use of the expression and offers up some eccentric formatting.

I won’t show any more excerpts, the story is that sometimes the ‘refactoring’ truly preserves the meaning of the code and sometimes it breaks it. It can be used in both tools but you need to think about it and check what the tool has done.

I started with a question: ‘Are we nearly there yet?’ the answer is a disappointing ‘nearly’.

References

[1] Fowler, Martin (1999) Refactoring: Improving the Design of Existing Code, Addison Wesley

[2] http://eclipse.org/cdt/

[3] https://www.cevelop.com/

[4] http://unity.ubuntu.com/mir/

Notes: 

More fields may be available via dynamicdata ..