4

I'm using an Optional class quite similar to that of boost. For semantic reasons, I switched an attribute of the same (structured) type in some class definitions (and therefore also in method signatures) from mandatory to optional. So, in each related class header file, I included the header with the definition of Optional and also a typedef to ease typing (see CHANGE comments); the passages read like this:

#include <MyAttrType.h>
#include <Optional.h>                          // CHANGE
typedef Optional<MyAttrType> OptionalAttrType; // CHANGE

class MyClassX
{
// ...

Today I run Cppcheck 1.68 on the project and got this style warning:

The typedef 'OptionalAttrType' hides a typedef with the same name.

How can I get rid of theses style warnings without making something bad?


Remarks:

  • MyAttrType itself is currently independent from Optional (because defined early in the project history, while Optional was being last time)
  • I think of introducing a new header only for this purpose. But maybe there are better options?
  • I guess that it could generally be an indication of bad design, to have many classes with the same optional attribute, but I'm not sure in my special case. The code is currently changing to reach higher prioritized goals.
  • of course, MyAttrType, OptionalAttrType, and MyClassX aren't the actual names
  • I think this is better suited for codereview.stackexchange.com – Tamás Szelei Jan 5 '15 at 12:56
  • 1
    @TamásSzelei I considered asking it there, but I cannot provide compilable code. I hoped this case could match a common pattern... – Wolf Jan 5 '15 at 13:01
  • What do you mean by compilable? I'm under the impression that codereview.SE is specifically for code that compiles and works fine but requires review. Isn't that true for your code? – Tamás Szelei Jan 5 '15 at 13:05
  • 3
    @TamásSzelei please read through Code Review's “on topic” guideline. On CR, you are expected to provide working real-world code to be reviewed. Short snippets and example code (which this is!) are not accepted. Reviews can be about anything that make the code better, e.g. idioms, formatting, or algorithms. OP doesn't seem to want a general review (“what could be improved here?”) but is trying to solve a concrete problem: “how can I get rid of this warning?” – amon Jan 5 '15 at 13:12
3

I typically use one of the two ways.

  1. First way is to typedef at the place-of-first-declaration.
  2. Second way is to typedef at each place-of-use, and make it only visible to that place-of-use (by putting it inside the class or method that uses it).

(1) Put the typedef close to the type that is being wrapped.

/* MyAttrType.h */

#include <Optional.h>

// README : See Optional<MyAttrType> at the end of this header

struct MyAttrType
{
    // ....
};

// (put the typedef here)
typedef Optional<MyAttrType> OptionalMyAttrType;

/* ---------------------------- */
/* Every other C++ source files */
#include "MyAttrType.h"

// .... Any code can use OptionalMyAttrType there.

(2) Make the typedef visible only to each class that uses it.

/* MyAttrType.h */

namespace my_pod_types
{
    struct MyAttrType { /* ... */ };
}

// (nothing else outside.)

/* ---------------------------- */
/* Every other C++ source files */

#include "MyAttrType.h"
#include "Optional.h"

class MyClassX
{
private:
    typedef Optional<my_pod_types::MyAttrType> OptionalMyAttrType;
public:
    // ...
private:
    OptionalMyAttrType m_optAttr;
};

Most of the time, the template relevant to me is either std::unique_ptr and std::shared_ptr. Furthermore, I will just decide that a class will either use one way or the other, and then typedef that smart pointer wrapper as "MyClassOnePtr". Draconian, but a library's main author is supposed to know what is the best for the library most of the time.

If it is not obvious which one of the two smart pointers should be preferred, then I will not put the typedef in the first header (so, I won't use option #1 unless the choice is obvious.)

Most of the time, you will realize that option #2 does not always shield the use of Optional<T> from the end-user. That is, the application logic may require users of MyClassX to deal with Optional<T> when interacting with it. When that happens, you can't hide it anymore. It is not an implementation detail; it is part of the visible surface.

Finally, you should be aware of the C++ limitation to forward-declare things. Namely, there are times where C++ needs to know up-front:

  • Size of the type being wrapped;
  • Existence of a default constructor and destructor;
  • And so on.

See this question on Stackoverflow (about unique::ptr) for examples of such limitations.


Another unrelated thing I would like to share, after reading ixrec's answer.

I started following a "nothing in the default namespace" policy after finding that you can't get Doxygen (a widely used C++ documentation to HTML generator) to generate well-organized documentation unless you categorize your classes by using namespaces. It seems it doesn't matter how you name those namespaces; as long as Doxygen sees them as distinct, and human users don't complain about it, any namespacing approach will be fine.

  • The first option seem to be what I've asked for, thx. – Wolf Apr 12 '15 at 20:37
0

If OptionalAttrType is part of the public API defined by those headers, then I would use some other name for your typedef.

If OptionalAttrType is an implementation detail that you aren't supposed to know about, then those headers should have put it inside a namespace to prevent collisions like this. Since they didn't, you have to work around this somehow.

Because you're asking this question, I assume you expect to encounter more of these collisions in the future, so you need a long-term solution. Unless you can change all the headers and source files you depend on to use namespaces or more consistent class names, the only general solution I can think of is choosing a prefix or suffix that's unlikely to collide with anything (eg, OptionalAttrType_TD) and start using it for all your typedefs.

A less general but perhaps more practical alternative is to simply hide this warning. Chapter 6 of the Cppcheck manual describes all the ways you can suppress a specific warning from that tool.

  • Thanks for the effort. Maybe it's harder to describe than I expected. The data-dominated attribute seemed to be essential when the using classes started to exist. After a paradigm change, this isn't the case any more. Placing the typedefs into namespaces seems not good, because they are really the same and the code duplicates would only be obscured. – Wolf Jan 6 '15 at 8:53
0

There are three errors:

  1. Your headers declare a name in a namespace not private to your library which they have no business declaring.
  2. You seem to not make sure there's only one definition of it in your whole library.
  3. You are trying to shut the warning up instead of correcting that bug.

What you should do is one of:

  1. Make it a part of your interface.
  2. Or hide it in your libraries namespace (probably in a nested namespace internal or such).
  3. Or remove it completely and writing the full names.

In case you choose option 1 or 2, make sure it's only defined in exactly one place.

  • Could you clarify it? – Wolf Apr 12 '15 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.