<div dir="ltr"><div>Even,</div><div><br></div><div>w.r.t. <a href="https://trac.osgeo.org/gdal/changeset/38344">https://trac.osgeo.org/gdal/changeset/38344</a><br></div><div><br></div><div>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.</div><div><br></div><div>1) C char * string vrs std::string vrs CPLString</div><div><br></div><div>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.</div><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><div><a href="http://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class">http://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class</a> </div><div><br></div><div>It's probably not worth discussing CPLString at the moment.</div><div><br></div><div>2) Why not put it all in the header so that the compiler can inline it all as it is so small?<br></div><div><br></div><div><br></div><div><br></div><div>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.</div><div><br></div><div><a href="https://gist.github.com/schwehr/a51330a3634dbb4d64a53a08673f0dd4">https://gist.github.com/schwehr/a51330a3634dbb4d64a53a08673f0dd4</a><br></div><div><br></div><div><div>// Totally untested.</div><div><br></div><div>class CPL_DLL CPLConfigOptionSetter</div><div>{</div><div>  public:</div><div>    CPLConfigOptionSetter(const std::string& pszKey, const std::string& osValue,</div><div>                          bool bSetOnlyIfUndefined) :</div><div>        m_osKey(osKey),</div><div>        m_osOldValue(CPLGetConfigOption(osKey.c_str(), "")),</div><div>        m_bRestoreOldValue(false)</div><div>    {</div><div>        if( (bSetOnlyIfUndefined &&</div><div>             CPLGetConfigOption(osKey.c_str(), NULL) == NULL) ||</div><div>            !bSetOnlyIfUndefined )</div><div>        {</div><div>            m_bRestoreOldValue = true;</div><div>            CPLSetThreadLocalConfigOption(osKey.c_str(), osValue.c_str());</div><div>        }</div><div>    }</div><div>    ~CPLConfigOptionSetter()</div><div>    {</div><div>        if( !m_bRestoreOldValue ) return;</div><div><br></div><div>        CPLSetThreadLocalConfigOption(m_osKey.c_str(), m_osOldValue.c_str());</div><div>    }</div><div><br></div><div>  private:</div><div>    const std::string m_osKey;</div><div>    const std::string m_osOldValue;</div><div>    bool m_bRestoreOldValue;</div><div><br></div><div>#if HAVE_CXX11</div><div>    CPLConfigOptionSetter(const CPLConfigOptionSetter&) = delete;</div><div>    CPLConfigOptionSetter& operator=(const CPLConfigOptionSetter&) = delete;</div><div>#else</div><div>    CPLConfigOptionSetter(const CPLConfigOptionSetter&);</div><div>    CPLConfigOptionSetter& operator=(const CPLConfigOptionSetter&);</div><div>#endif</div></div><div>}</div><div><br></div><div class="gmail_signature"></div>
</div>