shall be DEBUG

us |-> is

What's the rationale for this being so much less than MIN_FORECAST_AVAILABLE_DISK_SPACE?

I do not want to fill the disk no matter how large the models are. So I keep some room.

Note that MAX_FORECAST_MODEL_PERSISTANCE_MEMORY is actually the size in memory, not persistence. I haven't made tests yet, but I assume that writing it as json has some expansion factor. So the size on disk will be higher than the size in memory. I do not think we need to exactly calculate that but only make a worst case assumption.

500MB is totally educated guess, I wonder if we ever have forecast models of that size.

I will add some more code docs.

Are I see, that makes sense, ok cool plus we can always revisit if we think this is too conservative. +1 to a little more documentation on this.

missing \

Also, I wonder whether we should check to see if we actually restored the enum in this macro. I'm inclined to say we shouldn't and ditch restoreSuccess.

We generally don't bother to check if the field was missing altogether from the state document. Sometimes we can cope with default initialising a new field and it doesn't have to be present in the original state document, so this cuts down usability.

If you want to check what fields are present in the state document you can always fill in an unordered string set in the loop which does the restoring, i.e.

boost::unordered_set<std::string> restored;
do
{
    restored.insert(traverser.name());
    ...
}
while (traverser.next());
if (restored.count(MY_FIELD_I_WANT_TO_BE_PRESENT_TAG) > 0 && ...)
{
    return false;
}

FWIW: I first used a E_UNKNOWN state in the enum but that had to many side-effects on the existing code base. Other parts art not affected as I can initialize them to something invalid and check whether it has been set. So this only applies to enums, while enums are only used here.

For me the non-existence of the field is a bug.

I can use the suggested trick, but this looks more complicated to me than this 1 bool.

It definitely is more complicated for this case, but I want this macro to be as widely useable as possible.

Suppose we introduce a new enum member to an existing class, but feel we can happily get away with default initialising it to something. Then we don't want this extra "check a value exists". Now we could always go ahead and just ignore the bool, but that feels wrong.

That is the rationale for this suggestion, make the macro as general and simple as possible and if we need special handling deal with that elsewhere. As it is this feels like odd functionality out to have just this one case check for existence of a field.

Another option would be to have some general functionality which allows us to force that a list field values is actually read in a given loop. Something like registerManditoryFields on the traverser?

As per my usual comment, can we move to cc? Looks like these (at least FORECAST_MODEL_PERSIST_TAG) are only referenced in cc, so just defining them in unnamed namespace cuts down code and cleans up class definition.

Surely modelRestore can't be null if we get here (although see below).

Could we move into lambda to avoid code duplication also maybe modelRestore != nullptr.

that would be modelRestore.get() != nullptr, hmm operator bool is a valid C++ construct on auto pointers. I do not mind, but we should make a note of it and be consistent everywhere.

You don't need the .get(). There's an overload of the equality operator where rhs is nullptr_t: http://www.cplusplus.com/reference/memory/unique_ptr/operators/

We seem to generally be moving towards making the condition as explicit as possible, even using == false verses !, on grounds that it is easier to see the intention at a glance. Using != nullptr seems more consistent with that philosophy.

I am fine with that, lucene enforces this style as well.

There are some non-standard spaces between function name and argument lists in a few places, such as here. I guess once we've agreed on auto-formatting tooling this goes away, but would be nice not to introduce anything we won't accept in the meantime.

this->

Not something you introduced, but it feels like it would be nice to have a method on SModelParams to get this object. Also you've mixed brackets and braces and indentation levels, which makes this hard to read.

This line contains tabs.

I think in C++11 a number with no size suffix can be int, long or long long, whichever is the smallest it will fit into. But in C++98 it was only int or long. Visual Studio 2013 doesn't implement the complete C++11 standard, so this might cause an error when backporting, because long is still 32 bits on Windows. You could add ull to the number now or just watch out for a compilation problem on Windows when backporting to 6.x - Visual Studio 2013 might be OK with it as-is.

I think this should be wrapped in a try/catch in case it throws an unexpected exception.

Should be wrapped in try / catch. If it's a best effort thing then we can ignore the exception.

It would be good to include errorCode.message() in the output as well as the number. Otherwise it will be like the moon landings where users faced with "error 1202" have to contact mission control to find out what's actually gone wrong.

This line is indented with a tab.

Does CRestore only close its file in its destructor? If so, you may find this remove() fails on Windows because the file is still open. (Linux will let you delete an open file but Windows won't unless it was explicitly opened with share mode delete.) In other tests that have this problem we've put most of the code in an inner block so the appropriate destructor gets called before removing files.