Quantcast

[gdal-dev] Review of r38344 - CPLConfigOptionSetter

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[gdal-dev] Review of r38344 - CPLConfigOptionSetter

Kurt Schwehr-2
Even,


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.


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.


// 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
}


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review of r38344 - CPLConfigOptionSetter

Even Rouault-2

Kurt,

 

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

 

> 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?

 

This header doesn't include cpl_string.h, so CPLString isn't available. And there might be a subtle difference of behaviour between NULL and empty string in the previous value of the config option, that would be less convenient to take into account with std::string / CPLString

>

> 2) Why not put it all in the header so that the compiler can inline it all

> as it is so small?

>

>

 

I didn't see this performance critical to be really worth inlining.

 

Even

 

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Loading...