Create a shared dictionary class for plugin and engine

src/shared/Dictionary.cpp

+#include <fstream>
+#include <stdexcept>
+#include <Windows.h>
+
+#include "Dictionary.h"
+#include "Utils.h"
+
+Dictionary* Dictionary::instance = 0;
+
+const std::wstring baseLocale = L"en";
+
+Dictionary::Dictionary(const std::wstring& locale)
+{
+  std::wstring basePath = GetDllDir() + L"locales\\";
+
+  // Always load base locale first - that's our fallback
+  ReadDictionary(basePath, baseLocale);
+
+  // Now try to load by full locale code
+  if (locale != baseLocale && !ReadDictionary(basePath, locale))

[Felix Dahlke, 2013/06/11 12:31:14]

It looks like if this or the other ReadDictionary call below fail, some of the
data in the default dictionary may already have been overwritten. I assume
that's intended?

If it's unintended, we could avoid this and one unnecessary ReadDictionary call
by first attempting to load the dictionary for the full locale, then for the
short locale and only if all that fails would we load the default locale.

[Wladimir Palant, 2013/06/12 13:46:07]

Yes, it is intended - even incomplete localization is better than no
localization. Also, ReadDictionary was intentionally written in such a way that
it doesn't fail (trash is ignored). It returns false only if the input file
really cannot be opened.
Given that the idea here is to deal with incomplete locales - that would be a
bad idea.

[Felix Dahlke, 2013/06/13 18:07:37]

Okay, just wasn't sure if it was intended.

+  {
+    // Fall back to short locale name
+    size_t pos = locale.find(L'-');
+    if (pos != std::wstring::npos && locale.compare(0, pos, baseLocale) != 0)
+      ReadDictionary(basePath, locale.substr(0, pos));
+  }
+}
+
+void Dictionary::Create(const std::wstring& locale)
+{
+  if (!instance)
+    instance = new Dictionary(locale);
+}
+
+Dictionary* Dictionary::GetInstance()
+{
+  if (!instance)
+    throw std::runtime_error("Attempt to access dictionary before creating");
+
+  return instance;
+}
+
+bool Dictionary::ReadDictionary(const std::wstring& basePath, const std::wstring& locale)
+{
+  std::ifstream stream(basePath + locale + L".ini");

[Oleksandr, 2013/06/18 07:41:09]

I think it wouldn't hurt to use "filebuf::sh_read " just in case.

[Wladimir Palant, 2013/06/18 08:17:48]

Good point even though this is non-standard, will do that.

[Wladimir Palant, 2013/11/05 11:30:25]

After looking into this I am less convinced that this is a good idea. There is
no std::filebuf::sh_read as this parameter isn't part of STL. filebuf::sh_read
on the other hand requires including fstream.h (*not* fstream) and seems to be
kept for reasons of backwards compatibility only. As I see it, we shouldn't be
using it. Did I miss anything?

[Eric, 2013/11/05 13:56:06]

The full constructor is declared thus:

    explicit ifstream (const char* filename, ios_base::openmode mode =
ios_base::in);
    explicit ifstream (const string& filename, ios_base::openmode mode =
ios_base::in);

"ios_base::in" is the portable version of "filebuf::sh_read", and it's there by
default. As I recall, the "filebuf" extensions predate C++98, though I'm hazy on
that (of course).

The upshot: Default constructor argument is just fine.

[Wladimir Palant, 2013/11/06 08:51:46]

Not really. If you look at
http://msdn.microsoft.com/en-us/library/aa277521%28v=vs.60%29.aspx, we are
talking about different parameters here - filebuf::sh_read is "protection mode"
(file locking flag). Still, I think that read sharing is what we get for an
input stream by default.

[Eric, 2013/11/06 17:49:53]

After realizing that I hadn't remembered what was happening correctly, I looked
into it.

On 2013/11/06 08:51:46, Wladimir Palant wrote:
Apparently not. See
http://stackoverflow.com/questions/27700/c-opening-a-file-in-non-exclusive-mode
for some exposition on it.

There's no C++ standard way of ensuring this; that much I'm fairly sure of.
There's a slim possibility of failure if, say, two plugin instances try to run
ReadDictionary at the same time. The easiest way of dealing with it is to use
Oleksandr's original suggestion. It's not portable, but this code is not
currently being ported, either.

The GNU C++ library has non-standard exceptions to the ifstream constructor as
well (see http://www.math.utah.edu/docs/info/iostream_4.html). If we ever need a
portable solution, we'd need to make this parametric in some way. The cleanest
C++ was is with templates, but that solution is premature, because there's no
actual problem yet, just a potential future one.

I think what I'd do is to add the non-standard mode with a big comment about
what the goal is, which is essentially something like a "ifstream_readonly"
class.

[Felix Dahlke, 2013/11/06 23:09:50]

This seems a bit like premature optimisation to me - even if we don't have read
sharing, are we sure it's going to be a problem? Can we test that this is not
the default behaviour? Otherwise I wouldn't bother adding filebuf::sh_read. We
can still look into that if this turns out to be a performance bottleneck later.

[Wladimir Palant, 2013/11/07 06:53:57]

According to http://msdn.microsoft.com/en-us/library/vstudio/zek0beca.aspx
std::ifstream constructor also has a Microsoft-specific _Prot parameter
defaulting to ios_base::_Openprot. Looking inside xiosbase header where it is
defined I see the following:

#define _OPENPROT     _SH_DENYNO

Apparently, we could simply pass in some sharing flag from Share.h here.
However, the default is already the most permissive sharing mode which is
exactly what we need (the dictionary files are only being modified by the
installer and the application isn't running there). I guess I can close that
review then, we don't have to do anything.

+  if (stream.fail())
+    return false;
+
+  std::string section;
+  while (!stream.eof())

[Felix Dahlke, 2013/06/11 12:31:14]

I think the following would suffice, it'd be a bit less code:

std::string line;
while (std::getline(stream, line))
{
...

I'm not really sure if we need to check for stream.fail(), but I guess it won't
hurt.

[Wladimir Palant, 2013/06/12 13:46:07]

IMHO that will get us into an endless loop - std::getline always returns the
stream reference. Normally the streams won't even throw exceptions on failure
which is why checking stream.eof() and stream.fail() is necessary from all I
know.

[Felix Dahlke, 2013/06/13 18:07:37]

No, it wouldn't be an endless loop, this is the canonical way to use
std::getline. The operator void*() method of the returned stream reference is
called, which returns 0 when reading failed. Like I said, not even sure if
stream.fail() is still necessary if we do this.

It's true that streams don't throw exceptions, that's what stream.fail() checks
for. Although you can enable exceptions for streams:
http://stackoverflow.com/a/3180285/606036

+  {
+    std::string line;
+    std::getline(stream, line);
+    if (stream.fail())
+      return false;
+
+    line = ::TrimString(line);
+    if (line.size() >= 2 && line[0] == '[' && line[line.size() - 1] == ']')
+    {
+      // Section header
+      section = line.substr(1, line.size() - 2);
+    }
+    else if (line.size() >= 1 && line[0] == '#')

[Felix Dahlke, 2013/06/11 12:31:14]

This won't allow for comments preceeded by whitespace, or comments after text.

How about simply removing comments, like this?

std::string::size_type firstComment = line.find("#");
if (firstComment != std::string::npos)
  line = line.substring(0, firstComment);

[Wladimir Palant, 2013/06/12 13:46:07]

We call TrimLine earlier so whitespace is allowed. As to comments that aren't at
the beginning of the line, the following code got our subscriptionlist
maintainers in trouble (parsed by Python's configparser):

  [Section #1]

So I better don't make comments all too flexible.

[Felix Dahlke, 2013/06/13 18:07:37]

Hm. One benefit of this is that the translated strings could contain #. It's
fine.

+    {
+      // Comment
+      continue;
+    }
+    else
+    {
+      // Value
+      size_t pos = line.find('=');
+      if (pos != std::string::npos)
+      {
+        std::string key = ::TrimString(line.substr(0, pos));
+        std::string value = ::TrimString(line.substr(pos + 1));
+        data[KeyType(section, key)] = ToUtf16String(value);
+      }
+    }
+  }
+  return true;
+}
+
+std::wstring Dictionary::Lookup(const std::string& section, const std::string& key) const
+{
+  DataType::const_iterator it = data.find(KeyType(section, key));
+  if (it == data.end())
+    return L"### MISSING STRING [" + ToUtf16String(section) + L", " + ToUtf16String(key) + L"] ###";
+
+  return it->second;
+}