C++ Tips

This page collects various tips and common pitfalls when working with the C++ code. Some are generic, others are FlightGear specific

Primitive Types

• Use the SimGear primitive types for quaternions, vectors and matrices - the PLIB ones are deprecated (and PLIB will be removed at some point in the future). In some places it makes sense to use OSG types directly - that decision is left to the developer to decide which makes more sense.

• Use the boost helpers for string manipulation - they're more portable than C-library functions, especially on non-Unix systems.

• (for the future) Use the boost helpers for file, directory and path manipulation (instead of the PLIB equivalents). The main reason is that at some point in the future, PLIB will be be removed from the code. boost includes portable, robust wrappers around various useful file operations, such as iterating over members of a directory.

(someone needs to decide whether SGPath should become a wrapper for a boost file path, or just be replaced entirely)

Formatting and Style

Note FlightGear is full of different code styles and formats - the best guidance is to follow the code you're working in, unless it is obviously broken.

• Prefer early-return style. Don't make people scroll to see which code-path they're inside; use return to manage control flow, instead of deep nesting of if clauses. If you are nesting more than three level deep, consider making a helper function for the inner levels, or see if you can invert the logic to reduce indentation.

• Use exceptions. Exception support is not well developed, but they provide a clean way to handle genuinely unusual situations without (easily ignorable) bool or integer return codes. The main loop catches exceptions, so use the SimGear exception classes, provide good location and message strings when you throw, and all should be well. Using exceptions can greatly simplify code, by removing continual failure / error checks in the main code. Ensure you differentiate between fatal errors (use sg_error) and problems specific to a subsystem or operation (use sg_exception or some subclass).

• Ensure headers are independent and minimal (see the discussion of forward declarations further down). This means each header should include everything it needs to parse, and no more. Header should feature the following (in order):
  • an #ifndef guard
  • a one line comment stating the purpose of the file (or a full Doyxgen @file block if you prefer)
  • The GPL license boiler plate
  • The CVS revision tag
  • any required includes, ideally grouped by kind - for example standard C library, then standard C++ library, then SimGear, then other FlightGear headers
  • any forward declarations or namespaces required
  • ideally, exactly one class (which should match the name of the file). This rule is frequently broken in the existing code, and there are reasons not to obey it, but in general, simpler, standalone headers which define one class are much easier for developers, packagers, code indexers and compilers to deal with.

Headers should not feature a #ifdef HAVE_CONFIG_H block if at all possible (compiler portability sometimes defeats this). Source files should be including config.h as their first action, before including any real headers. Again, currently there are files in the codebase that do not follow this pattern.

• Source files should have the following structure:
  • A one line comment (or Doxygen @file section) describing the purpose of a file. If you're copying an existing file, please remember to update the comment
  • The GPL license boilerplate
  • The CVS revision tag
  • The #ifdef HAVE_CONFIG_H boilerplate include
  • An include of the header file corresponding to the source file. This provides an excellent test that the header file is independent - since it's the first thing included, that guarantees it correctly pulls in any other headers it may depend upon.
  • Any required includes, again grouped by kind as discussed for header files - generally 'simplest first'
  • Any namespace, forward declarations or using declarations. It's important that these occur after including all headers, to ensure the environment see by the headers is not polluted, which may conceal or cause problems.

• Respect the 80-column rule (and enable the right-hand margin in your editor if it supports one). This is not just pedantry, it makes comparing diffs side-by-side easier, or viewing code in other UI tools (eg debuggers). The most important reason is readability though - if your line is that long, perhaps you need to be using a typedef, wrapping some boolean logic in a predicate helper, or some other syntactic change.

• Break long functions and methods. Ideally, functions should fit on one screen - scrolling back up a function to check scope or variable naming harms readability. Sometimes longer methods do make sense, but the longer the method gets, the better your local variable naming needs to be.

• Typedef instantations of STL containers. This greatly aids readability, but also makes changing the definition much simpler - especially when using iterators, as is very common with the STL containers. It's much better to refer to IdNavaidMap::iterator than std::map<std::string, Navaid*>::iterator at each point you refer to an iterator type.

Standards

• Use SG_LOG calls instead of <cout> and iostreams. SG_LOG is controllable (partially so right now, moreso in the future) and the overhead of messages can be avoided at build time. <iostreams> can be expensive both at compile-time and run-time (not always, but if used naively).

• Use SGGeod instead of basic longitude/latitude/elevation doubles. It's easier to pass to other methods, type safe (can't confuse order of lat/lon) and unit-safe (degrees vs radians vs meters vs feet is explicit in the API).

• Never pollute the namespace in a header file, via using declarations, even for standard library types. I.e don't be lazy and write using std::string; in a header - each time you refer to string, vector, etc, you need to fully qualify the name with std::. It's a bit more to type, but much safer for code that includes the header file. (In source files, you are welcome to do as you please)

C++ Efficiency

Rather than duplicate the many fine works in this area, it's likely you should find and read 'Effective-C++', 'More Effective-C++' and 'Effective STL', all by Scott Meyers. The most critical points are repeated here, though.

• Always pass aggregate types by (const) reference, especially STL containers, string or mathematical tuples such as SGQuat or SGVec3. String copying in particular is common and leads to many wasteful malloc and free calls. Passing STL containers by value is extremely wasteful, as is returning them by value - pass them into methods by a reference, and out the same way. This avoids any container copying at all, hopefully.

• Prefer forward declaration to includes. Some things require pulling in a header, but in general headers should pull in everything the need to compile independently, and nothing more. Passing aggregate types by reference helps, since then a forward declaration suffices to refer to them. Avoid pulling in standard library headers (or system headers) into if you can avoid it, and never pull iostream into a header - use iosfwd, which exists for the purpose.

• The compiler is almost certainly smarter about inlining than you are. One line methods can live in header files, sometimes. Anything more should live in an implementation file. Otherwise, compile times increase for each compilation unit that includes the header. (Obviously templates are an exception to this)

Pitfalls

• Watch out for confusing geocentric coordinates (SGGeoc) and geodetic coordinates (SGGeod). If you use the SG types, the correct conversions are enforced, but many places in the code create a geocentric type from geodetic lat/lon. Geocentric computations are less expensive to compute, but there are very few places where the performance difference will be noticeable.

• Magnetic vs true headings, bearings and courses - watch out for whether any bearing values are true or magnetic. Most user-facing properties are magnetic, since that's what is used in charts, runway headings, etc. Internal computations (SGGeodesy::direct, etc) work in true bearings, so conversion is required. The magvar code computes magnetic variation for you, but also note the variation depends on position - if you read the variation from the property tree, you're getting the value at the user's latitude / longitude.

• Units - both imperial vs metric, elevation vs altitude, ground speed vs indicated airspeed, track vs heading, degrees vs radians, and so on. Naming can help, as can using types such as SGGeod to manage data. Many bugs can creep in by confusing similar but different values.

• When the simulation is paused, SGSubsystem::update is still called, but with dt = 0. This can easily lead to divide-by-zero errors in code; check for it explicitly if working on subsystem code that is doing time-dependency calculations.

• Do not catch all exceptions via catch (...) - this can mask real problems. Better granularity of exception handling does require some clean-ups to the hierarchy, so catchable / ignorable errors can be distinguished from fatal ones, but in the mean time, the safest policy is to let the main loop catch and report the exception, or to catch yourself and then re-throw.

• Make ownership of objects passed into / out of functions and methods explicit, where possible. This can be done via naming, documentation, use of shared pointers, ref-counted pointers or std::auto_ptr, to name a few possibilities. It's always better to ensure a method can only be called the 'right' way, rather than trying to document the intended behaviour and hope people respect it.