Issue #1234 - Remove 'CString' From 'ToLowerString()'

src/plugin/PluginUtil.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 35 matching lines @@
 std::wstring GetLocationUrl(IWebBrowser2& browser)
 {
   ATL::CComBSTR locationUrl;
   if (FAILED(browser.get_LocationURL(&locationUrl)) || !locationUrl)
   {
     return std::wstring();
   }
   return std::wstring(locationUrl, locationUrl.Length());
 }
 
-/**
- * \par Implementation Note
- *   We call '_wcslwr_s()', which take C-style strings and uses the current locale to determine case.

[sergei, 2015/12/16 15:53:16]

It still can be just a one line comment "uses the current locale" next to the
function call 

[Eric, 2015/12/16 16:38:07]

That's reasonable.

- */
 std::wstring ToLowerString(const std::wstring& s)
 {
   std::wstring lower(s); // Copy the argument
   /*
-   * C++11 provides that 'c_str()' returns the internal array in which the string holds its value.
-   * Thus we can modify in-place after casting away the 'const' modifier from 'c_str()'.

[sergei, 2015/12/16 15:53:16]

These two lines are completely redundant.

[Eric, 2015/12/16 16:38:07]

Just because you understand it already doesn't mean it doesn't clarify for
someone else.

You are not supposed to muck about this internals of 'std::basic_string'. We
are, and this comment explains it.

[Oleksandr, 2016/01/05 22:59:00]

From what I remember the consensus on comments is that we should in general aim
to make the code itself as readable as possible. Comments should be kept to
minimum because of that. In this instance I think one can figure out what's
going on without the comment, so I'd agree it would be best to remove these two
lines.

[Eric, 2016/01/07 14:34:04]

Done.

-   *
    * Documentation for '_wcslwr_s' https://msdn.microsoft.com/en-us/library/y889wzfw.aspx
    * This documentation is incorrect on an important point.
    * Regarding parameter validation, it says "length of string" where it should say "length of buffer".
    * The call below has argument "length + 1" to include the terminating null character in the buffer.

[sergei, 2015/12/16 15:53:16]

It does say "Size of the buffer.",  so these three lines can go away.

[Eric, 2015/12/16 16:38:07]

It also says "size of string", and it does that (like I said before) in the part
about validation. I quote:

| If numberOfElements is less than the length of the string, 
| the functions also return EINVAL and set errno to EINVAL.

When you pass 'numberOfElements' equal to the size of the string, you get an
exception.

Inconsistent documentation is incorrect documentation. The document is _in fact_
incorrect. Proper auditing the code requires a comment to correct the
"normative" documentation.

[Oleksandr, 2016/01/05 22:59:00]

How about just a comment like:
//NOTE: second parameter of _wcslwr_s expects length of buffer, not length of
string.

[Eric, 2016/01/07 14:34:04]

It's not typical for documentation to be incorrect. We should say so explicitly.

Leaving this one as is.

    */
-  _wcslwr_s(const_cast<wchar_t*>(lower.c_str()), lower.length() + 1);
+  auto e = _wcslwr_s(const_cast<wchar_t*>(lower.c_str()), lower.length() + 1); // uses the current locale
+  if (e != 0)
+  {
+    throw std::logic_error("Error code returned from _wcslwr_s()");
+  }
   return lower;
 }