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.
