IniReader template overrides become ambiguous

Question

I was tasked with refactoring our existing IniReader class, which is Windows only compatible, to be cross-platform ( specifically UNIX compatible ). We decided that boost::property_tree was a good fit for this and a quick play with property_tree confirms this.

The original class had many readValue overloads for the various data types. During the refactoring I was happy to discover that boost::property_tree::get_value is templatized so it made it simple to declare the following new readValue functions:

// read and return a value from the inifile 
// throws boost::property_tree:ptree_error if not found
template <class T> 
T readValue( const std::string& section, const std::string& param ) const;

// read and return value from section.param in the inifile
// if the param does not exists in the section return |defaultValue| 
template <class T> 
T readValue( const std::string& section, const std::string& param, const T& defaultValue ) const;

However, I later discover there's a couple of special overridden readValue functions which accept an additional int index. This allows values to be read using param name appended with the index value like this:

[TextFileFilters]
count=3
filter1=*.txt
filter2=*.csv
filter3=*.log
num1=50
num2=20
num3=10

Originally I thought of adding two more readValue overrides which includes the index argument:

// read and return a value from the inifile 
// throws boost::property_tree:ptree_error if not found
template <class T> 
T readValue( const std::string& section, const std::string& param, const int index ) const;

// read and return value from section.param in the inifile
// if the param does not exists in the section return |defaultValue| 
template <class T> 
T readValue( const std::string& section, const std::string& param, const T& defaultValue, const int index ) const;

BUT, there's a problem. When using T = int it becomes ambiguous to the compiler:

// is 3 the default int value OR the index ? 
int count = ini.readValue<int>( "TextFileFilters", "num", 3 ); 

What I've done, currently, is to remove the 3 parameter "indexed" override of readValue. Therefore mandating that all indexed readValue calls must be called with the default argument. This is not ideal.

The only thoughts around this I have was to introduce an new IniPath class, which attempts to replace the section, param or section, param, index combinations:

class IniPath
{
public:
  IniPath( const std::string& section, const std::string& path );
  IniPath( const std::string& section, const std::string& path, const int index );
};

...

template <class T> 
T readValue( const IniPath& path ) const;

template <class T> 
T readValue( const IniPath& path, T& defaultValue ) const;

but does that seems excessive? Too many temporaries?

int count = ini.readValue<int>( IniPath( "TextFileFilters", "count" ) ); 
int log_num = ini.readValue<int>( IniPath( "TextFileFilters", "num", 3 ) );

Do you have any suggestions or ideas?

Answer 1

I wouldn't worry too much about temporaries (shouldn't they be optimized away?), but I think you're focusing on the smaller problem. You fundamentally have the following options:

• Disambiguate by disallowing certain combinations, like you say you've currently done (readValue<T>(string, string), readValue<T>(string, string, T), and readValue<T>(string, string, int, T) but no readValue<T>(string, string, int))
• Disambiguate by changing parameters to the function (this could be by reordering non-template parameters e.g. readValue<T>(string, string, T) vs readValue<T>(string, int, string, T), or by the readValue<T>(IniPath, T) approach you describe where you make it someone else's problem)
• Disambiguate by changing function names, e.g. readValue<T>(string, string, T) vs. readIndexedValue<T>(string, string, int) (and of course readIndexedValue<T>(string, string, int, T))
• Disambiguate by changing function names the other way, e.g. readValue<T>(string, string) and readValue<T>(string, string, int) vs readValueDefault<T>(string, string, T) and readValueDefault<T>(string, string, int, T).

My opinion is that the first is too likely to create a pit of failure. Just like when you tried to add a non-defaulted readValue<T>(string, string, int) without immediately realizing the problem of doing so, users of your functions are likely to make the same mistake on the consumption side (hey, I've got readValue(sFoo, sBar) but now I need sBar 1, 2, and 3...) and wonder why things are compiling but not working correctly.

The second is okay; making it someone else's problem is a really good approach for some scenarios. Merely reordering the parameters like I mentioned can result in funky APIs like the deprecated parts of CRegKey::QueryValue where the order of parameters results in wildly different overloads that make the API hard to remember. I don't know whether this one is that bad, but I'm still predisposed against it.

The third is my favorite. It's simple, clean, and makes it easier to figure out what this extra int parameter is. The fourth is about as good, depending on whether indexes or defaults are more "natural", and of course you can combine both bullet's approaches.

But that's not the end of the story.

The bigger problem is this: You're migrating from existing usage with working code. You have to consider what the damage is, how to detect it, and how to fix it. Ideal case is things magically work 100%. Next best, in my opinion, is each call site either works the same as it used to or results in a compile-time error. Worse is a series of call sites you have to update, will find a pattern to do so, but sometimes the pattern is wrong (a change to IniPath might result in this). Worst by a long shot is call sites that change their meaning without any warning (you might be stuck here anyway if your call sites are ambiguous enough).

So do you have any call sites that will change meaning, and if so how will you address this? That will help determine how you disambiguate your function.