Hi John,
I agree - fixing fragility is a good thing. Can you please post a patch file for these changes and I can investigate applying when I have some time? Please remember not to use strcasecmp as it is not portable.
Thanks for contributing!
Scott
Great. Thanks.
While I'm at it, it seems to me that there is a not entirely
obvious dependency between mDescriptions[] in Log.cxx and
Log::Level in Log.hxx. That is, the position of the strings in
mDescriptions[] absolutely must correspond exactly with the values
defined in the Log::Level enum. Moreover, Log::Level is defined
with an #ifdef in the middle of it, and with various hardcoded
defines in it (stuff set equal to other symbols from linux header
files pulled in from elsewhere), and it all ends up compiling just
right to match up with mDescriptions[], both in linux and Windows.
So it works, but it is fragile in non-obvious ways.
The quick and dirty option would be to at least warn people with
comments. That is, a comment in Log.cxx saying "NOTE: This list is
order-dependent and MUST be kept in sync with the values defined
in Log::Level", and a corresponding comment in Log.hxx saying
"NOTE: The values defined here MUST be in sync with the order of
the strings in mDescriptions[]".
Even better would be to get rid of the dependence as outlined
below.
In Log.hxx, within class Log:
struct descriptionEntry
{
Level level;
const char * description;
};
...
static const descriptionEntry mDescriptions[];
Then, in Log.cxx:
const struct Log::descriptionEntry Log::mDescriptions[] =
{
{ None, "NONE" },
{ Crit, "CRIT" },
{ Crit, "CRITICAL" },
{ Err, "ERR" },
{ Err, "ERROR" },
{ Warning, "WARNING" },
{ Warning, "WARN" },
{ Info, "INFO" },
{ Debug, "DEBUG" },
{ Stack, "STACK" },
{ StdErr, "CERR" },
{ Bogus, NULL }
};
...
Data
Log::toString(Level l)
{
int i;
for (i = 0; mDescriptions[i].description; i++)
{
if (mDescriptions[i].level == l)
{
return log_ + mDescriptions[i].description;
}
}
return log_ + "INVALID";
}
Log::Level
Log::toLevel(const Data& l)
{
Data pri( l.prefix("LOG_") ? l.substr(4) : l);
int i = 0;
for (i = 0; mDescriptions[i].description; i++)
{
if (strcasecmp(pri.c_str(), mDescriptions[i].description)
== 0)
{
return mDescriptions[i].level;
}
}
cerr << "Choosing Debug level since string was not
understood: " << l << endl;
return Log::Debug;
}
Note that this includes the strcmp => strcasecmp change, and
note also that it maps several strings ("warn", "warning") to the
same error level, but while converting from a level back to a
string, it will grab the first one it hits, so the preferred
canonical string should be first in the struct (hence the
"WARNING" entry appears before "WARN").
-John Gregg
On 05/14/2014 11:29 AM, Scott Godin wrote:
I can't see a downside as well. Feel free to post
"patch" files to this list for inclusion in the project. This
one is very simple though - no need for a patch file. I can
apply it.
Note: strcasecmp is not available on all platforms - I
will use resip internal isEqualNoCase instead.
Thanks!
Scott
|