Case-insensitive string class

installer/src/installer-lib/process.h

Index: installer/src/installer-lib/process.h
===================================================================
--- a/installer/src/installer-lib/process.h
+++ b/installer/src/installer-lib/process.h
@@ -7,6 +7,8 @@
 
 #include "handle.h"
 
+#include <string>
+#include <cctype>
 #include <vector>
 #include <set>
 #include <algorithm>
@@ -15,42 +17,76 @@
 #include <Windows.h>
 #include <TlHelp32.h>
 
+//-------------------------------------------------------
+// wstring_ci: case-insensitive wide string
+//-------------------------------------------------------
 
-//-------------------------------------------------------
-// exe_name_set: case-insensitive wide-string set
-//-------------------------------------------------------
-int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ;
+/**
+ * Traits class for case-insensitive strings.
+ */
+template< class T >

[Wladimir Palant, 2014/03/31 05:58:35]

Style nit: excessive whitespace. There should be no whitespace after
opening/before closing angle bracket. Same elsewhere, not going to repeat.

+struct ci_traits: std::char_traits< T >
+{
+  static bool eq( T c1, T c2 )
+  {
+    return std::tolower( c1 ) == std::tolower( c2 ) ;
+  }
 
-struct exe_name
-{
+  static bool lt( T c1, T c2 )
+  {
+    return std::tolower( c1 ) < std::tolower( c2 ) ;
+  }
+
   /**
-   * Pointer to wide-character string, which is supposed to be null-terminated.
+   * Trait comparison function.
+   *
+   * Note that this is not a comparison of C-style strings.
+   * In particular, there's no concern over null characters '\0'.
+   * The argument 'n' is the minimum length of the two strings being compared.
+   * We may assume that the intervals p1[0..n) and p2[0..n) are both valid substrings.
    */
-  const wchar_t * name ;
-
-  exe_name( const wchar_t * name ) : name( name ) {} ;
-} ;
-
-template <>
-struct std::less< exe_name >
-  : std::binary_function< exe_name, exe_name, bool >
-{
-  bool operator()( const exe_name & a, const exe_name & b ) const
+  static int compare( const T * p1, const T * p2, size_t n )

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

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]

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.

[Eric, 2014/03/31 14:13:54]

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.

   {
-    return wcscmpi( a.name, b.name ) < 0 ; 
+    while ( n-- > 0 )

[Wladimir Palant, 2014/03/31 05:58:35]

Style nit: excessive whitespace. There should be no whitespace after
opening/before closing parentheses. Same elsewhere, not going to repeat.

+    {
+      T l1 = std::tolower( * p1 ++ ) ;
+      T l2 = std::tolower( * p2 ++ ) ;

[Wladimir Palant, 2014/03/31 05:58:35]

Style nit: excessive whitespace. There should be no space after * or before ++.
Same elsewhere, not going to repeat.

+      if ( l1 == l2 )
+      {
+	continue ;

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

Didn't notice it originally - please use spaces for indentation, not smart tabs.

[Eric, 2014/03/31 14:13:54]

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")

+      }
+      return ( l1 < l2 ) ? -1 : +1 ;
+    }
+    return 0  ;
   }
 } ;
 
-struct exe_name_set
-  : public std::set< exe_name >
+typedef std::basic_string< wchar_t, ci_traits< wchar_t > > wstring_ci ;
+
+
+//-------------------------------------------------------
+// file_name_set: case-insensitive wide-string set
+//-------------------------------------------------------
+struct file_name_set
+  : public std::set< wstring_ci >
 {
-  exe_name_set( const wchar_t * exe_name_list[], size_t n_exe_names )
+  /**
+   * Constructor initialization from an array.
+   */
+  template< size_t n_file_names >
+  file_name_set( const wchar_t * ( & file_name_list )[ n_file_names ] )

[Wladimir Palant, 2014/03/31 05:58:35]

Style nit: excessive whitespace. There should be no space after &.

[Wladimir Palant, 2014/03/31 05:58:35]

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.

[Eric, 2014/03/31 14:13:54]

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.

   {
-    for ( unsigned int j = 0 ; j < n_exe_names ; ++ j )
+    for ( unsigned int j = 0 ; j < n_file_names ; ++ j )
     {
-      insert( exe_name( exe_name_list[ j ] ) ) ;
+      insert( wstring_ci( file_name_list[ j ] ) ) ;
     }
   }
+
+  /**
+   * Empty set constructor has no arguments.
+   */
+  file_name_set()
+  {}

[Wladimir Palant, 2014/03/31 05:58:35]

Style nit: closing bracket should go on a new line.

 } ;
 
 //-------------------------------------------------------
@@ -61,10 +97,14 @@
 class process_by_any_exe_name_CI
   : public std::unary_function< PROCESSENTRY32W, bool >
 {
-  const exe_name_set & names ;
+  const file_name_set & names ;

[Wladimir Palant, 2014/03/31 05:58:35]

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.

[Eric, 2014/03/31 14:13:54]

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.

 public:
-  bool operator()( const PROCESSENTRY32W & ) ;
-  process_by_any_exe_name_CI( const exe_name_set & names )
+  bool operator()( const PROCESSENTRY32W & process )
+  {
+    return names.find( process.szExeFile ) != names.end() ;
+  }

[Wladimir Palant, 2014/03/31 05:58:35]

Nothing wrong with keeping implementations out of the header.

[Eric, 2014/03/31 14:13:54]

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.

+
+  process_by_any_exe_name_CI( const file_name_set & names )
     : names( names )
   {}
 } ;
@@ -82,20 +122,6 @@
 } ;
 
 /**
- * Filter by process name. Comparison is case-insensitive.
- */
-class process_by_name_CI
-  : public std::unary_function< PROCESSENTRY32W, bool >
-{
-  const wchar_t * name ;
-  const size_t length ;
-  process_by_name_CI() ;
-public:
-  bool operator()( const PROCESSENTRY32W & ) ;
-  process_by_name_CI( const wchar_t * name ) ;
-} ;
-
-/**
  * Extractor that copies the entire process structure.
  */
 struct copy_all
@@ -114,11 +140,6 @@
 } ;
 
 /**
- * Case-insensitive wide-character C-style string comparison, fixed-length
- */
-int wcsncmpi( const wchar_t * a, const wchar_t * b, unsigned int length ) ;
-
-/**
  * Retrieve the process ID that created a window.
  *
  * Wrapper around GetWindowThreadProcessId. 
@@ -431,7 +452,7 @@
    * The argument of the filter constructor is a set by reference.
    * Since it does not make a copy for itself, we define it as a class member to provide its allocation.
    */
-  exe_name_set exe_names ;
+  file_name_set process_names ;
 
   /**
    * Filter function object matches on any of the exe names specified in the constructor.
@@ -487,8 +508,9 @@
   } ;
 
 public:
-  Process_Closer( Snapshot & snapshot, const wchar_t * exe_name_list[], size_t n_exe_names )
-    : snapshot( snapshot ), exe_names( exe_name_list, n_exe_names ), filter( exe_names )
+  template< size_t n_process_names >
+  Process_Closer( Snapshot & snapshot, const wchar_t * ( & process_name_list )[ n_process_names ] )
+    : snapshot( snapshot ), process_names( process_name_list ), filter( process_names )
   {
     update() ;
   }