[sword-devel] SWConfig Changes

[David White]

On Thu, 2002-09-05 at 14:08, Troy A. Griffitts wrote:
> > - An additional constructor for SWConfig should be provided which takes
> > a sequence of configuration files, which are all loaded and combined
> > - A method should be provided which allows loading of a configuration
> > file and merging its contents with the current configuration information
> 
> I personally would like the constructor to take an SWConfig *, and the 
> augment method to take SWConfig *

Well the way I see it, a constructor taking an SWConfig* is essentially
a copy constructor. But I understand about the augment method taking a
SWConfig * (although I'd use a reference since it can't be null).

I assume you mean however, that the information is extracted out of the
passed SWConfig, and copied, rather than a reference to the SWConfig
object be maintained.

> 
> This way, if anyone decides to subclass SWConfig and make something 
> silly, like SWRegistryConfig, they could still use your MultiConfig dealeo.

sure, good point.

> 
> 
> > - When the Save method is called, each section is serialized to the file
> > it originally came from
> > - If a section exists in multiple files, then only those names which
> > came from that file are serialized to the file.
> > - If a name/value pair has been added since the files were loaded, then
> > that name/value pair is serialized to all files which contain the
> > relevant section. [1]
> 
> I think I prefer the [1] option.  Probably the LAST added SWConfig that 
> contained the section.  What will probably happen, is that something 
> like a systemwide module set will be loaded, then a 
> ~/.sword/userprefs.conf will be added to the config, which may override 
> any entries in the systemwide file.  Actually, it would be cool to 
> designate somehow that an SWConfig is readonly, or that an SWConfig is 
> the default write, in case a section doesn't exist, it would be created 
> in the default.

- the function to add a module takes a boolean flag which can set the
module to be read-only. The flag is false by default.
- if a module is read-only, then it will not be considered when changes
are to take place.

> 
> 
> 
> 
> > - When loading multiple files, if different files have the same name but
> > different values, for the same section, then both name/value pairs will
> > be loaded. If they have the same name and the same value, then only one
> > of the copies will be loaded.
> 
> Actually, there is another ambiguous situation that the current augment 
> code deals with.
> 
> consider:
> 
> file1.conf
> [MHC]
> GlobalOptionFilter=ThMLStrongs
> GlobalOptionFilter=ThMLMorph
> 
> file2.conf
> [MHC]
> GlobalOptionFilter=ThMLFootnotes
> 
> 
> In this case we want 3 entries in the final map
> 
> scenerio 2:
> 
> file1.conf
> [MHC]
> CipherKey=
> 
> file2.conf
> [MHC]
> CipherKey=ABCD1234EFGH5678IJKL
> 
> In this case we only want 1 entry.
> Current code handles this the best it can guess.

ok...

- a value is considered to be a "null" value if it is empty or consists
only of whitespace characters.
- if a key/value pair contains a null value, it is entered into the map
only if there are no other key/value pairs with the same key
- if a key/value pair is added with the same key as a key/value pair
that has a null value, then the existing key/value pair is removed.

> > - remove use of strtok(). strtok() is a non-reentrant function (not to
> > mention not thread safe), which is potentially dangerous, especially for
> > a library to use. Could be replaced by strtok_r(),
> 
> As long as whatever you use is fast and portable, I'm game, BUT know 
> that strtok is used all over the rest of the code, as well.

Well personally I think that using strtok() is an automatic bug, at
least if it's not documented. That's because strtok() has side effects,
and undocumented side effects can usually be considered bugs.

I have written a concrete example of misbehavior due to this. I must
admit I'm not familiar enough with the Sword API yet to comment on
whether my example seems contrived, but I hope it gets the point across.

Suppose you are a user, and you are storing a list of sword
configuration files, which you want to load in. You are storing them as
a comma-separated list, and now you want to parse the list, and load the
modules, putting them into a vector. So, you write code that looks like
this:

vector<SWConfig*> load_config(char* str)
{
	vector<SWConfig*> config;
	const char* p = strtok(str,",");
	do {
		config.push_back(new SWConfig(p));
		p = strtok(NULL,",");		
	} while(p);

	return config;
}

this code should work, but will fail, because the SWConfig constructor
will call SWConfig::Load, which will call strtok, which will clobber the
stored global pointer that strtok relies upon.

I can appreciate that this might not be considered a major or high
priority bug, but I do think it is a bug, and I would be happy to look
into replacing strtok() with an alternative (like my own tokenizer, that
I would make sure is just as fast as strtok()).

> 
> 
> > or hand-coded parsing
> > (since the parsing is not complicated)
> > - get rid of using namespace std; in the header file. Just because
> > people want to use Sword doesn't necessarily mean they want 1000+
> > symbols imported into the global namespace :)
> 
> great.  only add the using std::whatever you use.

well, I usually explicitly qualify names in header files, and then
either do the same in source files, or use using statements at the top
of the source file, below the headers.

> 
> 
> > - make multimapwithdefault more efficient by caching iterators
> 
> Just be sure it's understandable fast and portable.

naturally :)

> 
> 
> > - remove non-private member variables from the SWConfig
> 
> Please don't change the current interface.

ok. If it's ok though, I will add public accessor functions that will
allow users to use it as if the members were private.

> 
> 
> > - modify the code to handle arbitrary line-lengths.
> > 
> > Any comments on these suggested changes are welcome.
> 
> 
> I'm excited!  This functionality will be very coooool and useful!

I hope so!

-David.

> 
> 	-Troy.