[CMake] Re: Changing MD -> MT (+patch for free toolkit)

Mathieu Malaterre mathieu.malaterre at gmail.com
Fri Feb 22 05:19:25 EST 2008


On Tue, Feb 19, 2008 at 5:42 PM, Gonzalo Garramuño
<ggarra at advancedsl.com.ar> wrote:
> Mathieu Malaterre wrote:
>  > On Feb 19, 2008 5:43 AM, Gonzalo Garramuño <ggarra at advancedsl.com.ar> wrote:
>  >> Mathieu Malaterre wrote:
>  >>> Ok this was yet-another-weird-dll thingy, one cannot do:
>  >>>
>  >>> class GDCM_EXPORT String : std::string { ... }
>  >>>
>  >> Please, learn a little bit more about C++.
>  >
>  > Oh thank you ! I'll take your advice right away, mister expert. So
>  > could you please let me know why *in my particular case* this is
>  > wrong:
>  > 1. I did not add any data member
>  > 2. I did not overload the dstor
>  >
>
>  GDCM_EXPORT tells me you are using this in a library.
>
>  The reason this is wrong is this.  Say you write a function to get a
>  exension of a filename as uppercase:
>
>         GDCM_EXPORT MyString get_extension( const MyString& b )
>          {
>             MyString ext = b.to_upper();
>             // etc... do some other stuff like find '.' and substr
>             return ext;
>          }
>
>  Now... this will work.  But.... now func() can only be used with your
>  particular MyString class.  However, if you had done:
>
>          namespace str {
>             // to_upper implementation not shown
>             GDCM_EXPORT std::string to_upper( const std::string& b );
>
>             GDCM_EXPORT std::string get_extension( const std::string& b )
>             {
>               std::string ext = to_upper( b );
>               // etc... do some other stuff like find '.' and substr
>               return ext;
>             }
>          }

My gdcm::String is not a general purpose string, use std::string instead.

>  With this, the code for both to_upper() and get_extension() can be
>  safely be reused and works for *all* std::string objects.  You don't
>  force people to use your own string library.  This makes your library
>  more attractive and easier to reuse in other code.

I'll see about that 'attractiveness' in sf.net stats, thanks. As a
side note you do realize you are using a program that has exactly this
implementation, and seems to be to be quite popular.

>  Even, if you did not do 1 or 2, you might have done 3:
>         3. Yo might have overloaded (replaced) a string function.
>
>  class MyString : public std::string
>  {
>  public:
>     // silly example, but same applies to add, operator<<, etc.
>     size_type size() { return std::string::size() + 1; }
>  };
>
>  int main()
>  {
>   MyString* s = new MyString;
>   std::string* c = static_cast< std::string* >( s ); // correct
>   std::cerr << c->size() << std::endl;   // which size() gets called?
>                                          // not yours,
>                                          // but the one in std::string
>   return 0;
>  }
>
>  This example for 3) is very simple and obvious but in big programs this
>  issue may be more confusing as the distintion of what a pointer really
>  points to becomes more blurry.

I do not overload any member function, I only define operator, they do
not carry any RTTI info, or add function with non-existent signature
in superclass.

>  If you have only added non-conflicting functions in MyString to
>  std::string, what you have is safe but potentially not so beneficial.
>  You might either do as I showed and place your additional functions in a
>  namespace and keep using std::string everywhere (which makes your
>  library nice to use and probably popular) or create your own string
>  class, without deriving anything (and yes, you do need to create a lot
>  of code for doing a good string class -- it is C++ after all), or create
>  a proxy string class thru containment of std::string (which is like
>  creating your own string class from scratch, but a tad simpler):
>
>  class MyString
>  {
>  std::string str;
>
>  public:
>       typedef std::string::size_type size_type;
>       // other typedefs similar to std::string...
>
>  public:
>       MyString() {}
>       MyString(const MyString& b) : str(b.str) {}
>       MyString(const std::string& b) : str(b)  {}
>
>      // auto cast to string - optional (only if lazy, may not be wanted)
>      operator std::string() const { return str; }

You are doing a copy.

>      // dispatch size function
>      inline size_type size() const { return str.size(); }

you are missing the gazillion std::string member functions.

>      // my functions - not shown
>      void to_upper();
>
>      // .... dispatch others, etc...
>      // also remember to add operator=(const std::string& b)
>  };
>
>  This avoids you having to actually write your (potentially buggy) code

My code is 10 lines long, the most difficult part was to redo the typedef.

>  to the string functions, as you just dispatch the operations to the
>  std::string inside your class.  And the operator to std::string() and
>  copy constructor of std::string allows your class to interact with
>  std::string functions and elements rather seamlessly.  It is still
>  verbose, but not *THAT* much.  And the code is probably safer to use
>  through pointers and easy to extend without conflicts.

I'll make sure doxygen does not show the parent class of this
particular class, to avoid such hairy user misuse.

>  > Please do not copy some random comment found on the internet and
>  > instead comment on my particular implementation with the following 1
>  > and 2 restrictions. Thanks.
>  >
>
>  It is not some "random comment".  It is good design advice that sadly
>  applies to all C++ STL classes.
>
>  For more information, refer to a good C++ book. IIRCC, Scott Meyer's
>  Effective STL and Effective C++, covers this and other problems with all
>  the pitfalls in a very thorough fashion.

Ok not so random, but taken from a C++ book, right. I have taken good
notes of your advices, but -sadly- they do not apply to my limited
test case.

As a side note, this whole thread kills the only important thing: the
cmake part. My patch was lost in the middle of those comments, so I
uploaded it again on the cmake bug tracker:

  http://vtk.org/Bug/view.php?id=6417

On different subject I also uploaded a patch for FindJNI: make
interface more consistent with other module.

-- 
Mathieu


More information about the CMake mailing list