Case-insensitive string class

Case-insensitive string class. Eliminates separate C-style string comparison functions.

Converted constructors with array arguments to template versions with array-length arguments.

[Wladimir Palant, 2014-03-31 05:58:34]

Thank you, the code is much better this way. Most of the comments are style
nits: please use the same style for your code that we are using elsewhere as
well. Given that all the installer code is already using the wrong style, I'm
fine if these are fixed in a separate change after this one lands.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
File installer/src/custom-action/close_application.cpp (right):

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:25: static const wchar_t *
engine_names[ 1 ] ;
Style nit: excessive whitespace. There should be no whitespace between * or
inside square brackets. Also, there should be no space before a semicolon. Same
elsewhere, not going to repeat.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:29: : snapshot(), ie_closer(
snapshot, ie_names ), engine_closer( snapshot, engine_names )
Style nit: excessive whitespace. There should be no space before or after a
parameter list (same elsewhere, not going to repeat).

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
File installer/src/installer-lib/process.h (right):

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:27: template< class T >
Style nit: excessive whitespace. There should be no whitespace after
opening/before closing angle bracket. Same elsewhere, not going to repeat.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:50: while ( n-- > 0 )
Style nit: excessive whitespace. There should be no whitespace after
opening/before closing parentheses. Same elsewhere, not going to repeat.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:53: T l2 = std::tolower( * p2 ++ ) ;
Style nit: excessive whitespace. There should be no space after * or before ++.
Same elsewhere, not going to repeat.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:77: file_name_set( const wchar_t * ( &
file_name_list )[ n_file_names ] )
Style nit: excessive whitespace. There should be no space after &.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:77: file_name_set( const wchar_t * ( &
file_name_list )[ n_file_names ] )
We might want to have a non-templated version of this constructor as well, just
in case it is being initialized from a memory buffer rather than an array.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:89: {}
Style nit: closing bracket should go on a new line.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:100: const file_name_set & names ;
Please don't use references as class members, create a copy. You cannot know
when the caller of your constructor is going to destroy its copy.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:105: }
Nothing wrong with keeping implementations out of the header.

[Oleksandr, 2014-03-31 07:03:03]

The part which eliminates string lengths parameter is pretty much what I have as
well (http://codereview.adblockplus.org/6003395731128320/). Would be a
conflicting commit, I suppose. Also, I don't quite see a point of having the
case-insensitive string class, as I said in the comment below.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
File installer/src/installer-lib/process.h (right):

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:48: static int compare( const T * p1,
const T * p2, size_t n )
Once again, I think it would be much easier to just use _wcsnicmp
(http://msdn.microsoft.com/en-US/library/chd90w8e.aspx) instead. Seems to me
like we are just inventing a wheel for the 3rd time. This is Microsoft specific
code anyway..

[Wladimir Palant, 2014-03-31 09:33:34]

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
File installer/src/installer-lib/process.h (right):

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:48: static int compare( const T * p1,
const T * p2, size_t n )
That's the standard way of doing this actually. When choosing between the
standard way and Microsoft-specific way I prefer the former unless there is a
good reason why it is suboptimal. This string class will also work with std::set
properly so we won't need any hacks there.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:56: continue ;
Didn't notice it originally - please use spaces for indentation, not smart tabs.

[Eric, 2014-03-31 14:13:53]

On 2014/03/31 05:58:34, Wladimir Palant wrote:
> Thank you, the code is much better this way.

No problem. This change was already on my personal list of things to get to. I
think the only difference we might have had is how important it was and where to
prioritize it.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
File installer/src/installer-lib/process.h (right):

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:48: static int compare( const T * p1,
const T * p2, size_t n )
On 2014/03/31 07:03:03, Oleksandr wrote:
> much easier to just use _wcsnicmp

It would work, obviously, since that's what we were doing earlier. It would also
be more efficient, since we're not copying string data to construct a C++
string. Today's code is comparing fixed strings (initialized from static array)
with essentially-fixed strings (C-string in the PROCESSENTRY32w etc. struct),
and so C-style string comparison does not create any hazard at the present time.

Using the C++ idiom makes the code clearer and shorter. It is also more robust
against changes by people who might not take the time to understand how the code
works.

The upshot of this change is that we are weighing maintainability and clarity
over efficiency and use of a pre-written function. If we had a serious
efficiency issue, I might argue differently, but this code runs plenty fast for
its application already.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:56: continue ;
On 2014/03/31 09:33:34, Wladimir Palant wrote:
> Didn't notice it originally - please use spaces for indentation, not smart
tabs.

That's Visual Studio acting up again. I thought I had fixed this in my install,
but apparently I was remembering fixing this for some other IDE.

For reference, here's the precis of how to fix it in VS:
1. Menu bar >> Tools >> Options
2. Dialog >> Text Editor >> C/C++ >> Tabs >> Insert spaces (radio button group
with "Keep tabs")

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:77: file_name_set( const wchar_t * ( &
file_name_list )[ n_file_names ] )
On 2014/03/31 05:58:35, Wladimir Palant wrote:
> We might want to have a non-templated version of this constructor as well

We're also getting an empty-set constructor for this class. If these class were
getting initialized from dynamic data, it's easier just to instantiate it empty
and insert the elements directly. After all, this class is a simple derivation
from std::set.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:100: const file_name_set & names ;
On 2014/03/31 05:58:35, Wladimir Palant wrote:
> You cannot know
> when the caller of your constructor is going to destroy its copy.

In this case, we can. This class is entirely subordinate to class
Process_Closer, which has both an instance of this filter class and the
file_name_set instance. One reason it's a class member of Process_Closer is
because of the refresh() function there, which can reuse the object. It could be
instantiated in Process_Closer.update() each time, but I think it's better that
they both appear in the Process_Closer constructor initialization together, to
show that there's not actually a lifetime issue.

As an algorithm object, it'll get instantiated in order to run some associated
function, and its useful lifetime is just that of the call. For the most part,
references are not hazardous in this case, since it's the caller that owns the
referent. 

If this were general-purpose library code, these arguments wouldn't apply,
because you don't know what the user is going to do with your class. But that's
not the case here.

I've added an explanatory comment on the member variable on this issue.

http://codereview.adblockplus.org/4790070691233792/diff/5629499534213120/inst...
installer/src/installer-lib/process.h:105: }
On 2014/03/31 05:58:35, Wladimir Palant wrote:
> Nothing wrong with keeping implementations out of the header.

No, there's not. There's nothing wrong with putting them into the header these
days, either, though, particularly for a one-liner like this one. And with
template code (which must be in headers) increasingly common, together with much
better optimizers, I've shrugged off my old reluctance on this issue.

I moved it into the header with this revision because I found it read better
when I revisited the code for this change.

[Eric, 2014-07-18 16:44:19]

See review for "Whole installer"
http://codereview.adblockplus.org/6202981292703744/