[gdal-dev] Review of r38344 - CPLConfigOptionSetter

Kurt Schwehr schwehr at gmail.com
Sun May 14 15:43:32 PDT 2017


Even,

w.r.t. https://trac.osgeo.org/gdal/changeset/38344

It might be a good time to ask you a couple of style questions.  It would
be good to have an explanation of why you chose to implement it the way you
did.  I aim for code that is of the same style for chunks that are both
small and large...  So some of these things might seem a little strange
only in the context of this commit.

1) C char * string vrs std::string vrs CPLString

Why did you choose a char *?  This function is trivial a C++ style string
would add little overhead to GDAL overall.  C strings are inherently touchy
things and using them means you must be careful with the destructor.  The
reader must also read the dtor to check if you did it right.  Bare allocs
(CPLStrdup in this case) are giant red flags for readers like me.  If you
used std::string for m_pszKey and m_pszOldValue, you can always use c_str()
to get a short term C const char * string.

I'm not a fan of CPLString, so I'm glad you didn't use that.  But all over
the code I see the use of CPLString when a std::string would work just
fine.  And if I remember correctly, I've seen you do this before.  Why
didn't you use a CPLString here?

We could always add helper functions to provide all of the CPLString
functionality without having to derive a new class on top of std::string.

http://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class


It's probably not worth discussing CPLString at the moment.

2) Why not put it all in the header so that the compiler can inline it all
as it is so small?



So something like this is pretty compact and has no bare allocs/news or
free/deletes.  It does call CPLGetConfigOption twice, which is a bummer.

https://gist.github.com/schwehr/a51330a3634dbb4d64a53a08673f0dd4

// Totally untested.

class CPL_DLL CPLConfigOptionSetter
{
  public:
    CPLConfigOptionSetter(const std::string& pszKey, const std::string&
osValue,
                          bool bSetOnlyIfUndefined) :
        m_osKey(osKey),
        m_osOldValue(CPLGetConfigOption(osKey.c_str(), "")),
        m_bRestoreOldValue(false)
    {
        if( (bSetOnlyIfUndefined &&
             CPLGetConfigOption(osKey.c_str(), NULL) == NULL) ||
            !bSetOnlyIfUndefined )
        {
            m_bRestoreOldValue = true;
            CPLSetThreadLocalConfigOption(osKey.c_str(), osValue.c_str());
        }
    }
    ~CPLConfigOptionSetter()
    {
        if( !m_bRestoreOldValue ) return;

        CPLSetThreadLocalConfigOption(m_osKey.c_str(),
m_osOldValue.c_str());
    }

  private:
    const std::string m_osKey;
    const std::string m_osOldValue;
    bool m_bRestoreOldValue;

#if HAVE_CXX11
    CPLConfigOptionSetter(const CPLConfigOptionSetter&) = delete;
    CPLConfigOptionSetter& operator=(const CPLConfigOptionSetter&) = delete;
#else
    CPLConfigOptionSetter(const CPLConfigOptionSetter&);
    CPLConfigOptionSetter& operator=(const CPLConfigOptionSetter&);
#endif
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20170514/c55ba38d/attachment.html>


More information about the gdal-dev mailing list