Skip to content

Commit

Permalink
Reduce usage of property regex matching (#2122)
Browse files Browse the repository at this point in the history 
Updates the generated property code to include a boolean indicating if the property uses regular expressions for matching. It also removes the case sensitivity check on the property names in favor of a check on the prefix.

Now, if a property does not require regex we just do a plain string comparison.

Closes #2108 and #2106
  • Loading branch information
@externl
externl authored May 7, 2024
1 parent bc4582b commit 776437e
Show file tree
Hide file tree
Showing 15 changed files with 4,442 additions and 4,315 deletions.
70 changes: 47 additions & 23 deletions config/makeprops.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@
struct Property
{
const char* pattern;
bool usesRegex;
const char* defaultValue;
bool deprecated;
const char* deprecatedBy;
Property(const char* n, const char* dv, bool d, const char* b) :
Property(const char* n, bool r, const char* dv, bool d, const char* b) :
pattern(n),
usesRegex(r),
defaultValue(dv),
deprecated(d),
deprecatedBy(b)
Expand Down Expand Up @@ -242,7 +244,9 @@ def closeFiles(self):
"""Needs to be overridden in derived class"""
pass

def propertyImpl(self, propertyName, defaultValue, deprecated, deprecatedBy):
def propertyImpl(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
"""Needs to be overridden in derived class"""
pass

Expand All @@ -262,9 +266,13 @@ def handleNewSection(self, sectionName, noCmdLine):
self.sections.append(sectionName)
self.newSection()

def handleProperty(self, propertyName, defaultValue, deprecated, deprecatedBy):
def handleProperty(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
self.properties[propertyName] = deprecatedBy if deprecatedBy else ""
self.propertyImpl(propertyName, defaultValue, deprecated, deprecatedBy)
self.propertyImpl(
propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
)

def startElement(self, name, attrs):
if name == "properties":
Expand Down Expand Up @@ -304,10 +312,13 @@ def startElement(self, name, attrs):
if c.isPrefixOnly():
return

usesRegex = "[any]" in propertyName
deprecatedBy = attrs.get("deprecatedBy", None)
deprecated = attrs.get("deprecated", "false").lower() == "true"
defaultValue = attrs.get("default", None) or ""
self.handleProperty(propertyName, defaultValue, deprecated, deprecatedBy)
self.handleProperty(
propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
)

def endElement(self, name):
if name == "properties":
Expand Down Expand Up @@ -372,10 +383,14 @@ def closeFiles(self):
def fix(self, propertyName):
return propertyName.replace("[any]", "*")

def propertyImpl(self, propertyName, defaultValue, deprecated, deprecatedBy):
propertyLine = 'IceInternal::Property("{section}.{name}", {defaultValue}, {deprecated}, {deprecatedBy})'.format(
section=self.currentSection,
name=self.fix(propertyName),
def propertyImpl(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
name = f"{self.currentSection}.{propertyName}"

propertyLine = 'IceInternal::Property("{pattern}", {usesRegex}, {defaultValue}, {deprecated}, {deprecatedBy})'.format(
pattern=self.fix(name) if usesRegex else name,
usesRegex="true" if usesRegex else "false",
defaultValue=f'"{defaultValue}"',
deprecated="true" if deprecated else "false",
deprecatedBy=f'"{deprecatedBy}"' if deprecatedBy else "nullptr",
Expand Down Expand Up @@ -455,10 +470,13 @@ def fix(self, propertyName):
#
return propertyName.replace(".", r"\\.").replace("[any]", r"[^\\s]+")

def propertyImpl(self, propertyName, defaultValue, deprecated, deprecatedBy):
line = 'new Property("{section}\\\\.{name}", {defaultValue}, {deprecated}, {deprecatedBy})'.format(
section=self.currentSection,
name=self.fix(propertyName),
def propertyImpl(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
name = f"{self.currentSection}.{propertyName}"
line = 'new Property("{pattern}", {usesRegex}, {defaultValue}, {deprecated}, {deprecatedBy})'.format(
pattern=self.fix(name) if usesRegex else name,
usesRegex="true" if usesRegex else "false",
defaultValue=f'"{defaultValue}"',
deprecated="true" if deprecated else "false",
deprecatedBy=f'"{deprecatedBy}"' if deprecatedBy else "null",
Expand Down Expand Up @@ -529,10 +547,13 @@ def closeFiles(self):
def fix(self, propertyName):
return propertyName.replace(".", r"\.").replace("[any]", r"[^\s]+")

def propertyImpl(self, propertyName, defaultValue, deprecated, deprecatedBy):
line = 'new Property(@"^{section}\\.{name}$", {defaultValue}, {deprecated}, {deprecatedBy})'.format(
section=self.currentSection,
name=self.fix(propertyName),
def propertyImpl(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
name = f"{self.currentSection}.{propertyName}"
line = 'new(@"{pattern}", {usesRegex}, {defaultValue}, {deprecated}, {deprecatedBy})'.format(
pattern=f"^{self.fix(name)}$" if usesRegex else name,
usesRegex="true" if usesRegex else "false",
defaultValue=f'"{defaultValue}"',
deprecated="true" if deprecated else "false",
deprecatedBy=f'"{deprecatedBy}"' if deprecatedBy else "null",
Expand Down Expand Up @@ -595,11 +616,14 @@ def closeFiles(self):
def fix(self, propertyName):
return propertyName.replace(".", "\\.").replace("[any]", ".")

def propertyImpl(self, propertyName, defaultValue, deprecated, deprecatedBy):
def propertyImpl(
self, propertyName, usesRegex, defaultValue, deprecated, deprecatedBy
):
if self.currentSection in self.validSections:
line = 'new Property("^{section}\\.{name}$", {defaultValue}, {deprecated}, {deprecatedBy})'.format(
section=self.currentSection,
name=self.fix(propertyName),
name = f"{self.currentSection}.{propertyName}"
line = 'new Property("{pattern}", {usesRegex}, {defaultValue}, {deprecated}, {deprecatedBy})'.format(
pattern=f"^{self.fix(name)}" if usesRegex else name,
usesRegex="true" if usesRegex else "false",
defaultValue=f'"{defaultValue}"',
deprecated="true" if deprecated else "false",
deprecatedBy=f'"{deprecatedBy}"' if deprecatedBy else "null",
Expand Down Expand Up @@ -657,10 +681,10 @@ def handleNewSection(self, sectionName, cmdLine):
f.handleNewSection(sectionName, cmdLine)

def handleProperty(
self, propertyName, default=None, deprecated=False, deprecatedBy=None
self, propertyName, usesRegex, default, deprecated, deprecatedBy
):
for f in self.handlers:
f.handleProperty(propertyName, default, deprecated, deprecatedBy)
f.handleProperty(propertyName, usesRegex, default, deprecated, deprecatedBy)

def startElement(self, name, attrs):
for f in self.handlers:
Expand Down
4 changes: 2 additions & 2 deletions cpp/include/IceUtil/StringUtil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ namespace IceUtilInternal
// UTF8 string/characters but ignore non ASCII characters. Unlike, the
// C methods, these methods are not local dependent.
//
ICE_API std::string toLower(const std::string&);
ICE_API std::string toUpper(const std::string&);
ICE_API std::string toLower(std::string_view);
ICE_API std::string toUpper(std::string_view);
ICE_API bool isAlpha(char);
ICE_API bool isDigit(char);

Expand Down
103 changes: 54 additions & 49 deletions cpp/src/Ice/Properties.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,69 +29,74 @@ namespace
// Check if the property is legal.
LoggerPtr logger = getProcessLogger();
string::size_type dotPos = key.find('.');
if (dotPos != string::npos)

// If the key doesn't contain a dot, it's not a valid Ice property.
if (dotPos == string::npos)
{
string_view prefix = key.substr(0, dotPos);
for (int i = 0; IceInternal::PropertyNames::validProps[i].properties != 0; ++i)
{
string pattern{IceInternal::PropertyNames::validProps[i].properties[0].pattern};
return nullopt;
}

dotPos = pattern.find('.');
string_view prefix = key.substr(0, dotPos);

//
// Each top level prefix describes a non-empty
// namespace. Having a string without a prefix followed by a
// dot is an error.
//
assert(dotPos != string::npos);
string propPrefix = pattern.substr(0, dotPos);
// Find the property list for the given prefix.
const IceInternal::PropertyArray* propertyArray = nullptr;

// If the prefix doesn't match, continue to the next prefix.
if (IceUtilInternal::toUpper(propPrefix) != IceUtilInternal::toUpper(string{prefix}))
{
continue;
}
for (int i = 0; IceInternal::PropertyNames::validProps[i].properties != nullptr; ++i)
{
string_view pattern{IceInternal::PropertyNames::validProps[i].properties[0].pattern};
dotPos = pattern.find('.');

for (int j = 0; j < IceInternal::PropertyNames::validProps[i].length; ++j)
{
const IceInternal::Property& prop = IceInternal::PropertyNames::validProps[i].properties[j];
// Each top level prefix describes a non-empty
// namespace. Having a string without a prefix followed by a
// dot is an error.
assert(dotPos != string::npos);
string_view propPrefix = pattern.substr(0, dotPos);

if (IceUtilInternal::match(string{key}, prop.pattern))
{
if (prop.deprecated && logWarnings)
{
logger->warning("deprecated property: " + string{key});
}
return prop;
}
if (propPrefix == prefix)
{
// We've found the property list for the given prefix.
propertyArray = &IceInternal::PropertyNames::validProps[i];
break;
}

// Check for case-insensitive match.
// As a courtesy to the user, perform a case-insensitive match and suggest the correct property.
// Otherwise no other warning is issued.
if (logWarnings && IceUtilInternal::toLower(propPrefix) == IceUtilInternal::toLower(prefix))
{
ostringstream os;
os << "unknown property prefix: `" << prefix << "'; did you mean `" << propPrefix << "'?";
return nullopt;
}
}

if (IceUtilInternal::match(
IceUtilInternal::toUpper(string{key}),
IceUtilInternal::toUpper(prop.pattern)))
{
if (logWarnings)
{
ostringstream os;
os << "unknown property: `" << key << "'; did you mean `" << prop.pattern << "'";
logger->warning(os.str());
}
return nullopt;
}
}
if (!propertyArray)
{
// The prefix is not a valid Ice property.
return nullopt;
}

if (logWarnings)
for (int i = 0; i < propertyArray->length; ++i)
{
auto prop = propertyArray->properties[i];

bool matches = prop.usesRegex ? IceUtilInternal::match(string{key}, prop.pattern) : key == prop.pattern;

if (matches)
{
if (prop.deprecated && logWarnings)
{
ostringstream os;
os << "unknown property: `" << key << "'";
logger->warning(os.str());
logger->warning("deprecated property: " + string{key});
}
return nullopt;
return prop;
}
}

// The key does not match a known Ice property
if (logWarnings)
{
ostringstream os;
os << "unknown property: `" << key << "'";
logger->warning(os.str());
}
return nullopt;
}

Expand Down
Loading

0 comments on commit 776437e

Please sign in to comment.