Introduce AsyncURIMatcher class to encapsulate the URI matching logic with and without regex support (-D ASYNCWEBSERVER_REGEX=1)

[mathieucarbou]

This PR wil replace #299

This pull request introduces two new example sketches and a comprehensive README for demonstrating and testing the advanced URI matching capabilities of the ESPAsyncWebServer library, specifically focusing on the AsyncURIMatcher class. The changes provide both a user-friendly demonstration (URIMatcher.ino) and a thorough test suite (URIMatcherTest.ino) covering all matching strategies, including factory methods, case-insensitive matching, and regex support. The new documentation in README.md explains matching behavior, usage patterns, and real-world applications.

New Example and Test Sketches

• Added examples/URIMatcher/URIMatcher.ino: A demonstration sketch showcasing all matching strategies supported by AsyncURIMatcher, including exact, prefix, folder, extension, case-insensitive, regex, and combined flag matching. Includes a navigable HTML homepage and detailed Serial output for each route.
• Added examples/URIMatcherTest/URIMatcherTest.ino: A test suite for validating all matching modes, including factory methods, case-insensitive matching, regex, and catch-all routes. Designed for automated testing with external scripts.

Documentation and Usage Guide

• Added examples/URIMatcher/README.md: A comprehensive guide explaining the AsyncURIMatcher class, auto-detection logic, matching strategies, usage patterns, available flags, performance notes, and real-world application examples.

Demonstration Features

• Demonstrates traditional string-based routing (auto-detection), explicit matcher syntax, factory functions, and combined flags for flexible routing. [1] [2]
• Shows how to enable and use regular expression matching via the ASYNCWEBSERVER_REGEX compilation flag. [1] [2]

Testing Enhancements

• The test sketch includes coverage for all matcher types, case-insensitive variants, regex patterns, and a catch-all POST handler, ensuring robust validation of the URI matching subsystem.

References: [1] [2] [3]

[mathieucarbou]

@willmmiles @me-no-dev : quick update. I have pushed again to remove URIMatcher support for static file handler because I saw when trying to write examples that supporting regex is useless unless we completely change the way this thing is implemented. it requires a non regex path to find the file on disk.

That being say, this leaves us with 2 handlers supporting regex:

1. json / message pack
2. default one

I do not think this is worth supporting regex for WebSocket and EventSource which currently match the exact URI.

So for these 2 use cases, we can easily put in common the way to match requests because json handler is just a specialization of the default one handling automatically a specific content type.

When not using regex, In main, json handler matches:

1. if uri length is 0
2. if uri == request.url (exact match)
3. if request.url starts with uri + / (folder match like request to /foo/bar will match a handler set at /foo)

When not using regex, In main, the default handler matches:

1. if uri length is 0
2. if uri == request.url (exact match)
3. if request.url starts with uri + / (folder match like request to /foo/bar will match a handler set at /foo)
4. if uri starts with /*., matches any request url ending with the end => this is used to match any file extension of any path. For example, /*.zip would match /foo/bar.zip, /foo.zip.
5. if uri ends with *, we match any request url starting with the same characters before the star. So * would match anything, /foo* would match /foobar and /foo/bar`, etc

So 1,2,3 are all common, no worry for them.

4 and 5 are cases applied (checked) before 3 in the default handler.

3 looks weird to me. I would have expected a handler to be set at /foo/ for a folder match. But /foo with match /foo and /foo/bar.

CONCLUSION:

I have updated the matcher:

    // empty URI matches everything
    if (!_value.length()) {
      return true;
    }

    String path = request->url();
    if (_ignoreCase) {
      path.toLowerCase();
    }

    // exact match (should be the most common case)
    if (_value == path) {
      return true;
    }

    // wildcard match with * at the end
    if (_value.endsWith("*")) {
      return path.startsWith(_value.substring(0, _value.length() - 1));
    }

    // prefix match with /*.ext
    // matches any path ending with .ext
    // e.g. /images/*.png will match /images/pic.png and /images/2023/pic.png but not /img/pic.png
    if (_value.startsWith("/*.")) {
      return path.endsWith(_value.substring(_value.lastIndexOf(".")));
    }

    // finally check for prefix match with / at the end
    // e.g. /images will also match /images/pic.png and /images/2023/pic.png but not /img/pic.png
    if (path.startsWith(_value + "/")) {
      return true;
    }

    // we did not match
    return false;

I think this is backward compatible (to be tested of course). Matches the current behavior of default handler and adds supprot for * and /* matching in json one.

I also added ignoreCase support.

About rewrites:

I initially had the idea to apply the logic to rewrites but we cannot since it won't be backward compatible for 3 things

• the rewrite from() method requires to keep the uri string, so if we use regex, this is 2 fields to keep in memory - we cannot do optimizations
• the rewrite checks for equality, which is not working because a matcher set at /foo will match /foo/bar for example
• removing a rewrite can be done by passing a string currently matching the from, but depending how we implement our URIMatcher, testing for equality would either require access some value field kept or allow some weird behavior regarding ignoreCase.

About memory usage

I know some users have A LOT of endpoints. With this new class, a handler will always have an instance of URIMatcher. So this is really important that we do not extend the memory footprint too much by addign too many fields.

So if we want to support regex and ignore case, it means we have to use different classes, eventually overrides with a combination of templates, like the ones suggested by Will above.

[willmmiles]

I have pushed again to remove URIMatcher support for static file handler because I saw when trying to write examples that supporting regex is useless unless we completely change the way this thing is implemented. it requires a non regex path to find the file on disk.

On the one hand, changing the way AsyncStaticWebHandler was implemented to directly support more sophisticated matching was, more-or-less, what I had in mind to leverage regex matching support. The basic approach I'd thought of was to apply match substitution on _path -- eg "\1" would get replaced with _pathParams[0], etc.: this allows fairly powerful file name construction and manipulation.

However! I can also make an argument that any such path matching voodoo can be done today through AsyncCallbackWebHandler, and often with cleaner and more efficient code (with or without regex). Unfortunately, that also means that right now we don't get all the cache handling, etc. from AsyncStaticWebHandler. It might be simpler to address that directly by factoring the cache header logic out to something that can be called independently. (Some future PR.)

TL;DR: I concur that we can leave AsyncStaticWebHandler as is for now.

if uri length is 0
if uri == request.url (exact match)
if request.url starts with uri + / (folder match like request to /foo/bar will match a handler set at /foo)
if uri starts with /., matches any request url ending with the end => this is used to match any file extension of any path. For example, /.zip would match /foo/bar.zip, /foo.zip.
if uri ends with , we match any request url starting with the same characters before the star. So * would match anything, /foo would match /foobar and /foo/bar`, etc

I was very much hoping to start moving towards explicit match behavioural flags in the new matcher, and away from magic URL string syntax. Particularly since some of those cases probably should be mutually exclusive: the folder-suffix case is particularly dangerous when I mean to require only an exact match. on("/file.html",...) probably doesn't expect to match "/file.html/whoa_there/what_is_this".

I was thinking something like:

class AsyncURIMatcher {
public:
  enum MatchFlags {
    MatchAll = (1<<0),
    MatchExact = (1<<1),           // matches equivalent to regex: ^{_uri}$
    MatchPrefixFolder = (1<<2),    // matches equivalent to regex: ^{_uri}/.*
    MatchPrefixAll = (1<<3),       // matches equivalent to regex: ^{_uri}.*
    MatchExtension = (1<<4),       // matches equivalent to regex: \.{_uri}$
#ifdef ASYNCWEBSERVER_REGEX
    MatchRegex = (1<<5),           // matches _url as regex
#endif
    
    MatchAuto = (1<<30),           // parse _uri at construct time and infer match type(s)
                                   // (_uri may be transformed to remove wildcards)
    MatchCaseInsensitive = (1<<31)
  };

  AsyncURIMatcher(String uri, int32_t flags = MatchAuto);

  // ...
  bool matches(AsyncWebServerRequest* request) {
    if (_flags & MatchAll)  return true;

    String path = request->url();
    if (_flags & MatchCaseInsensitive) {
      path.toLowerCase();
    }

    // exact match (should be the most common case)
    if ((_flags & MatchExact) && (_uri == path)) {
        return true;
    }
    // .. etc
  }
}

Does that make sense? Then users can construct exactly the match logic they want, state is kept to a minimum, and we can also keep "Auto" for backwards compatibility as "parse the uri like we always did". (Also we don't have to re-parse the uri for matching on every request!)

[mathieucarbou]

Does that make sense? Then users can construct exactly the match logic they want, state is kept to a minimum, and we can also keep "Auto" for backwards compatibility as "parse the uri like we always did". (Also we don't have to re-parse the uri for matching on every request!)

I have some changes locally, not matching that but going in nearly the same direction I suppose. What I saw when continuing using one urimatcher class is that it will increase the memory used for apps having a lot of handlers. So I was searching for a way to avoid that and found no alternative but to use an interface with 3 implementations: case sensitive one, case incentive one and regex one.

i overloaded the on() methods also.

another alternative would be to keep the current behavior for const char* uri parans but allow some different Marc pattern depending on the parameter.

I really try in my local changes to not have a uri object with more than 1 field because it would mean more memory usage.

[willmmiles]

I really try in my local changes to not have a uri object with more than 1 field because it would mean more memory usage.

No matter how we slice it, offering any kind of match type options will require us to store what they are. Using overloading means we pay for storing what match logic to run with a vtable pointer instead of a flags member, but we still pay for it. Taken to the limit (ie. explicitly specified matching semantics) we might also end up paying for it with more code space instead as well. Plus we'd have to switch to using indirection to the AsyncURIMatcher instance in the Handler class, so we would also pay the cost for yet another pointer.

If memory size is the biggest concern, I'd go with a flags word and re-constructing the std::regex every time - it'll be the smallest solution that still supports explicit match type selection.

Although vtables do also have the advantage that we might be able to arrange the regex implementation so as to avoid needing the #ifdef to save code space (ie. it only links the regex stuff if you're using it).

Storing the constructed std::regex is really CPU<->RAM tradeoff; if RAM usage is the biggest concern, then we can do without it like we have thus far. I would argue that explicit match type selection is more about correctness, and I'm not sure I'd want to compromise on that, though.

But! If we really want to get in to the realm of RAM optimization, one technique would be to overload the flags word as a std::regex pointer by using a bit that will never appear in a valid pointer (typically bit 0, as most objects are at least word aligned). If bit 0 is set, it's flags; if bit 0 is not set, it's an std::regex<> pointer, and we skip the usual analysis and just call the regex code. Small-string optimization classes like std::string or String often do something like this.

[mathieucarbou]

But! If we really want to get in to the realm of RAM optimization,

I agree we have to decide on a tradeoff. For example, I was looking to introduce a case insensitive matcher. A lot of websites are case insensitive. And just that requires a boolean to be stored with the uri value if we do not use polymorphism.

For example if we go without an interface, how users would be able to implement a case insensitive match without regexp ? Or even if we do not and one day we do want case insensitive match, then we have an object with 2 fields eventually where we try to now put a third one.

I know we pay a cost in every solution, but to me it seems that if there is a cost to pay somewhere, then be it in a way that things can be extensible, not closed, and open for future evolution ?

I will try to push this evening where I was with these changes regarding inheritance so that you can see the idea. I went there because I was not able within 1 UriMatcher object to support regex + standard match + case insensitive match and now like discussed above eventually several flavors of match (which is then another flag on top of that).

At the end users could ask the moon, like we do now, that's why maybe providing an interface could be an option.

[willmmiles]

But! If we really want to get in to the realm of RAM optimization,

I agree we have to decide on a tradeoff. For example, I was looking to introduce a case insensitive matcher. A lot of websites are case insensitive. And just that requires a boolean to be stored with the uri value if we do not use polymorphism.

For example if we go without an interface, how users would be able to implement a case insensitive match without regexp ? Or even if we do not and one day we do want case insensitive match, then we have an object with 2 fields eventually where we try to now put a third one.

With the sketch above, all the various options, including case sensitivity, were packed in to a single 32-bit flag member, with many bits to spare. That solution could cover all cases with a total cost of 1 DWORD per Handler and no significant runtime cost.

I know we pay a cost in every solution, but to me it seems that if there is a cost to pay somewhere, then be it in a way that things can be extensible, not closed, and open for future evolution ?

Because we pay for that extensibility today, even if we're not using it yet. Using a vtable solution has a minimum additional cost of 4 DWORDs per Handler-- one in the handler object itself (AsyncURIMatcher _uri must become AsyncURIMatcher* _uri - we have to store the pointer to the object, plus the object itself); one in AsyncURIMatcher to store the vtable; and two for the heap metadata (object size and next pointer). So 4x the RAM cost -- plus also the CPU cost of dynamic dispatch and the extra pointer dereferences; and the code space cost to store all those vtables, constructors, destructors, etc. etc. It adds up fast. :(

I will try to push this evening where I was with these changes regarding inheritance so that you can see the idea. I went there because I was not able within 1 UriMatcher object to support regex + standard match + case insensitive match and now like discussed above eventually several flavors of match (which is then another flag on top of that).

Sounds good. I'll see if I can sketch the solution I'm proposing above - it's really not that far from where you're at with 9ef3718.

[mathieucarbou]

Sounds good. I'll see if I can sketch the solution I'm proposing above - it's really not that far from where you're at with 9ef3718.

Thanks!

I have updated #310 and rebased this PR on top of it.

I am happy with the PR as it is because it is simple and supports regex + url matching like before + case insensitive.

The only thing I really don't like is this big AsyncURIMatcher object. I know users have more than a hundred of endpoints (don't ask me why), but it means they will be subject to some decrease of ram with so many AsyncURIMatcher objects in memory, especially if they are using regex for 1 or 2 endpoints they end up having 98 unused regex objects.

So that's why I tried today a polymorphism version, that I've pushed in branch https://github.com/ESP32Async/ESPAsyncWebServer/tree/urimatcher-poly.

In this branch pretty much only the URIMatcher classes change plus the on() flavors:

I ended up with this hierarchy to limit the quantity of fiels, but like you say it also introduce an overhead for the polymorphism. But this is a quite constant overhead right ? For users having a LOT of handlers, I guess this is still better than having hundreds of unused regex or boolean objects ?

class AsyncURIMatcher {
public:
  AsyncURIMatcher() {}
  AsyncURIMatcher(const AsyncURIMatcher &) = default;
  AsyncURIMatcher(AsyncURIMatcher &&) = default;
  virtual ~AsyncURIMatcher() = default;
  AsyncURIMatcher &operator=(const AsyncURIMatcher &) = default;
  AsyncURIMatcher &operator=(AsyncURIMatcher &&) = default;
  virtual bool matches(AsyncWebServerRequest *request) const { return false; };
};

class AsyncCaseSensitiveURIMatcher : public AsyncURIMatcher {
public:
  AsyncCaseSensitiveURIMatcher() {}
  AsyncCaseSensitiveURIMatcher(const char *uri) : _value(uri) {}
  AsyncCaseSensitiveURIMatcher(String uri) : _value(std::move(uri)) {}
  AsyncCaseSensitiveURIMatcher(const AsyncCaseSensitiveURIMatcher &) = default;
  AsyncCaseSensitiveURIMatcher(AsyncCaseSensitiveURIMatcher &&) = default;
  virtual ~AsyncCaseSensitiveURIMatcher() override = default;
  AsyncCaseSensitiveURIMatcher &operator=(const AsyncCaseSensitiveURIMatcher &) = default;
  AsyncCaseSensitiveURIMatcher &operator=(AsyncCaseSensitiveURIMatcher &&) = default;
  virtual bool matches(AsyncWebServerRequest *request) const override {
    return pathMatches(request->url());
  }

protected:
  String _value;

  bool pathMatches(const String &path) const {
    // empty URI matches everything
    if (!_value.length()) {
      return true;
    }

    // exact match (should be the most common case)
    if (_value == path) {
      return true;
    }

    // wildcard match with * at the end
    if (_value.endsWith("*")) {
      return path.startsWith(_value.substring(0, _value.length() - 1));
    }

    // prefix match with /*.ext
    // matches any path ending with .ext
    // e.g. /images/*.png will match /images/pic.png and /images/2023/pic.png but not /img/pic.png
    if (_value.startsWith("/*.")) {
      return path.endsWith(_value.substring(_value.lastIndexOf(".")));
    }

    // finally check for prefix match with / at the end
    // e.g. /images will also match /images/pic.png and /images/2023/pic.png but not /img/pic.png
    if (path.startsWith(_value + "/")) {
      return true;
    }

    // we did not match
    return false;
  }
};

class AsyncCaseInsensitiveURIMatcher : public AsyncCaseSensitiveURIMatcher {
public:
  AsyncCaseInsensitiveURIMatcher() : AsyncCaseSensitiveURIMatcher() {}
  AsyncCaseInsensitiveURIMatcher(const char *uri) : AsyncCaseSensitiveURIMatcher(uri) {
    _value.toLowerCase();
  }
  AsyncCaseInsensitiveURIMatcher(String uri) : AsyncCaseSensitiveURIMatcher(std::move(uri)) {
    _value.toLowerCase();
  }

  bool matches(AsyncWebServerRequest *request) const override {
    String path = request->url();
    path.toLowerCase();
    return AsyncCaseSensitiveURIMatcher::pathMatches(path);
  }
};

#ifdef ASYNCWEBSERVER_REGEX
class AsyncRegexURIMatcher : public AsyncURIMatcher {
public:
  AsyncRegexURIMatcher() {}
  AsyncRegexURIMatcher(std::regex pattern) : _pattern(std::move(pattern)) {}
  AsyncRegexURIMatcher(const char *uri, bool ignoreCase = false) : _pattern(ignoreCase ? std::regex(uri, std::regex::icase) : std::regex(uri)) {}
  AsyncRegexURIMatcher(String uri, bool ignoreCase = false) : _pattern(ignoreCase ? std::regex(uri.c_str(), std::regex::icase) : std::regex(uri.c_str())) {}

  AsyncRegexURIMatcher(const AsyncRegexURIMatcher &) = default;
  AsyncRegexURIMatcher(AsyncRegexURIMatcher &&) = default;
  ~AsyncRegexURIMatcher() = default;

  AsyncRegexURIMatcher &operator=(const AsyncRegexURIMatcher &) = default;
  AsyncRegexURIMatcher &operator=(AsyncRegexURIMatcher &&) = default;

  bool matches(AsyncWebServerRequest *request) const {
    std::smatch matches;
    std::string s(request->url().c_str());
    if (std::regex_search(s, matches, _pattern)) {
      for (size_t i = 1; i < matches.size(); ++i) {
        request->_pathParams.emplace_back(matches[i].str().c_str());
      }
      return true;
    }
    return false;
  }

private:
  std::regex _pattern;
};
#endif

I'll wait and see what you can come up with :-)

What I find nice with the polymorphism version is that we can easily provide a DSL:

on(caseMatch("/foo"), [](...) {...});
on(iCaseMatch("/foo"), [](...) {...});
on(regexMatch("^/foo$"), [](...) {...});
on(starMatch("/foo*"), [](...) {...});
on(extMatch("/*.png"), [](...) {...});
on(subMatch("/dir/"), [](...) {...});
...

[mathieucarbou]

We have some backward compatibility failures around extension matching. Looking at them right now.

Commit pushed. Condition was wrong: } else if (_value.lastIndexOf("/*.") > 0) { => } else if (_value.lastIndexOf("/*.") >= 0) {

All tests pas now:

Now adding some for the new API...

Still a remaining bug regarding All matcher fixed in c554794

~/.../examples/URIMatcherTest ( urimatcher → origin {4} ✓ )
❯  ./test_routes.sh 
Testing URI Matcher at http://192.168.4.1:80
==================================
Testing routes that should work (200 OK):
Testing /status ... ✅ PASS (200)
Testing /exact ... ✅ PASS (200)
Testing /exact/ ... ✅ PASS (200)
Testing /exact/sub ... ✅ PASS (200)
Testing /api/users ... ✅ PASS (200)
Testing /api/data ... ✅ PASS (200)
Testing /api/v1/posts ... ✅ PASS (200)
Testing /files/document.pdf ... ✅ PASS (200)
Testing /files/images/photo.jpg ... ✅ PASS (200)
Testing /config.json ... ✅ PASS (200)
Testing /data/settings.json ... ✅ PASS (200)
Testing /style.css ... ✅ PASS (200)
Testing /assets/main.css ... ✅ PASS (200)

Testing AsyncURIMatcher factory methods:
Testing /factory/exact ... ✅ PASS (200)
Testing /factory/prefix ... ✅ PASS (200)
Testing /factory/prefix-test ... ✅ PASS (200)
Testing /factory/prefix/sub ... ✅ PASS (200)
Testing /factory/dir/users ... ✅ PASS (200)
Testing /factory/dir/sub/path ... ✅ PASS (200)
Testing /factory/files/doc.txt ... ✅ PASS (200)
Testing /factory/files/sub/readme.txt ... ✅ PASS (200)

Testing case insensitive matching:
Testing /case/exact ... ✅ PASS (200)
Testing /CASE/EXACT ... ✅ PASS (200)
Testing /Case/Exact ... ✅ PASS (200)
Testing /case/prefix ... ✅ PASS (200)
Testing /CASE/PREFIX-test ... ✅ PASS (200)
Testing /Case/Prefix/sub ... ✅ PASS (200)
Testing /case/dir/users ... ✅ PASS (200)
Testing /CASE/DIR/admin ... ✅ PASS (200)
Testing /Case/Dir/settings ... ✅ PASS (200)
Testing /case/files/doc.pdf ... ✅ PASS (200)
Testing /CASE/FILES/DOC.PDF ... ✅ PASS (200)
Testing /Case/Files/Doc.Pdf ... ✅ PASS (200)

Testing special matchers:
Testing POST /any/path (all matcher) ... ✅ PASS (200)

Checking for regex support...
Regex support detected - testing traditional regex routes:
Testing /user/123 ... ✅ PASS (200)
Testing /user/456 ... ✅ PASS (200)
Testing /blog/2023/10/15 ... ✅ PASS (200)
Testing /blog/2024/12/25 ... ✅ PASS (200)
Testing AsyncURIMatcher regex factory methods:
Testing /factory/user/123 ... ✅ PASS (200)
Testing /factory/user/789 ... ✅ PASS (200)
Testing /factory/blog/2023/10/15 ... ✅ PASS (200)
Testing /factory/blog/2024/12/31 ... ✅ PASS (200)
Testing /factory/search/hello ... ✅ PASS (200)
Testing /FACTORY/SEARCH/WORLD ... ✅ PASS (200)
Testing /Factory/Search/Test ... ✅ PASS (200)

Testing routes that should fail (404 Not Found):
Testing /nonexistent ... ✅ PASS (404)
Testing /factory/exact/sub ... ✅ PASS (404)
Testing /factory/dir ... ✅ PASS (404)
Testing /factory/files/doc.pdf ... ✅ PASS (404)
Testing /exact ... ✅ PASS (200)
Testing /EXACT ... ✅ PASS (404)
Testing /user/abc ... ✅ PASS (404)
Testing /blog/23/10/15 ... ✅ PASS (404)
Testing /factory/user/abc ... ✅ PASS (404)

==================================
Test Results:
✅ Passed: 54
❌ Failed: 0
Total: 54

🎉 All tests passed! URI matching is working correctly.

Good to go!

[willmmiles]

If I may offer one more suggestion: I'm not sure we really need the bitfield-nature of the type enum anymore. It might be simpler and clearer to convert it to a basic counting enum instead, use a switch-case for the handling. willmmiles@b873a7e What do you think?

[mathieucarbou]

If I may offer one more suggestion: I'm not sure we really need the bitfield-nature of the type enum anymore. It might be simpler and clearer to convert it to a basic counting enum instead, use a switch-case for the handling. willmmiles@b873a7e What do you think?

I will try tomorrow.

During testing I saw that we cannot have any enum leading to 0 because this case is overruled by Nonregex (1) so this case becomes unavailable when we activate regex but do not use it.

That's why I've re-ordered the enum and set numbers starting at 1 and not 0.

[willmmiles]

During testing I saw that we cannot have any enum leading to 0 because this case is overruled by Nonregex (1) so this case becomes unavailable when we activate regex but do not use it.

That's why I've re-ordered the enum and set numbers starting at 1 and not 0.

In willmmiles@b873a7e, I explicitly bit-shifted the enum type when regex mode is available to reserve the LSB for Nonregex. Otherwise we have to ensure that the enum values count by two (so the LSB is left available for Nonregex).

That said, there's some micro-benchmarking possible there to find out which is faster ...

enum Type {
  // Compiler assigns values starting at zero
  Alpha,
  Beta,
  Gamma,
   // etc.
};

#ifdef REGEX
Type type = static_cast<Type>(_matchData >> 1);
#else
Type type = static_cast<Type>(_matchData);
#endif

switch(type) {
   case Alpha:
      return ...;
   case Beta:
      return ...;
   case Gamma:
      return ...;
};

or

#ifdef REGEX
#define ENUMVAL(i) ((i<<1) + 1)    // have to shift the value so we never get an even number
#else
#define ENUMVAL(i) i
#endif

enum ExpandedType {
  // Values include nonregex bit
  Alpha = ENUMVAL(0),
  Beta = ENUMVAL(1),
  Gamma = ENUMVAL(2),
   // etc.
};

switch(static_cast<ExpandedType>(_matchData)) {
   case Alpha:
      return ...;
   case Beta:
      return ...;
   case Gamma:
      return ...;
};

[mathieucarbou]

During testing I saw that we cannot have any enum leading to 0 because this case is overruled by Nonregex (1) so this case becomes unavailable when we activate regex but do not use it.
That's why I've re-ordered the enum and set numbers starting at 1 and not 0.

In willmmiles@b873a7e, I explicitly bit-shifted the enum type when regex mode is available to reserve the LSB for Nonregex. Otherwise we have to ensure that the enum values count by two (so the LSB is left available for Nonregex).

That said, there's some micro-benchmarking possible there to find out which is faster ...

enum Type {
  // Compiler assigns values starting at zero
  Alpha,
  Beta,
  Gamma,
   // etc.
};

#ifdef REGEX
Type type = static_cast<Type>(_matchData >> 1);
#else
Type type = static_cast<Type>(_matchData);
#endif

switch(type) {
   case Alpha:
      return ...;
   case Beta:
      return ...;
   case Gamma:
      return ...;
};

or

#ifdef REGEX
#define ENUMVAL(i) ((i<<1) + 1)    // have to shift the value so we never get an even number
#else
#define ENUMVAL(i) i
#endif

enum ExpandedType {
  // Values include nonregex bit
  Alpha = ENUMVAL(0),
  Beta = ENUMVAL(1),
  Gamma = ENUMVAL(2),
   // etc.
};

switch(static_cast<ExpandedType>(_matchData)) {
   case Alpha:
      return ...;
   case Beta:
      return ...;
   case Gamma:
      return ...;
};

I didn’t notice the shift ! That’s why I was wondering 🙂
I personnally prefer the second version because all the magic happens during enum definition and we control it - is is private. So it even does not matter if the values are changing.

What is important is to always have the same values for enums for the public flag enum type.

[mathieucarbou]

@willmmiles : I pushed 03e1c74

I went a little further to improve the code readability and maintenance and avoid potential mistakes in the future.

    // Matcher types are internal, not part of public API
  enum class Type {
    None,                 // default state: matcher does not match anything
    Auto,                 // parse _uri at construct time and infer match type(s): _uri may be transformed to remove wildcards
    All,                  // matches everything
    Exact,                // matches equivalent to regex: ^{_uri}$
    Prefix,               // matches equivalent to regex: ^{_uri}.*
    Extension,            // non-regular match: /pattern../*.ext
    BackwardCompatible,   // matches equivalent to regex: ^{_uri}(/.*)?$
    Regex,                // matches _url as regex
  };

• went with enum class to have a real type to improve type safety checks
• removed the "technical" enum types like NonRegex
• introduced the None instead of Default which is not matching anything: I think this is better to use None than to match all
• I am doing the bit shift operation always, in order to have a constant behavior and catch mistakes earlier, like the one where a modifier flag would be set a the msb
• I have also re-ordered the match operation: in steps: first extract the type, then modifier, then apply modifiers, then do the matching. This is a little less performant because I am not checking the All case upfront for example, and checking the modifier even if not potentially needed, but I think it is best for readability and to understand the code. This is not a big loss.

[mathieucarbou]

@me-no-dev @willmmiles : PR is squashed and rebased and is ready for a final review and to be merged. If this is possible for you, please sooner than later so that we can have this change stay in main branch ASAP before doing a release, so that all people pointing to main can test it.

Thanks :-)

[willmmiles]

Sorry there are some old comments mixed in this review, I can't see them on the code anymore. I'll close them once it posts.

[mathieucarbou]

@willmmiles : thanks a lot for the review. I have applied the fixed and also added a commit (2d63a43) to remove the Auto enum. Now, the enum class Type is completely isolated from the bit shift magic and only holds enum matching different match types. Auto was a hacky way to notify the constructor that it had to determine the match type, but this is only valid for the public constructor not using the factory methods.

This will be easier to maintain this way since someone wanting to add a new match type does not have to care about the under the hood bit shift magic.

update: moved your comments to unresolved so that you can quickly identify them.

[willmmiles]

Sorry to keep asking more questions.

[willmmiles]

This should be constexpr so the compiler can pre-calculate the static values in the functions above.

[mathieucarbou]

Yes, done in c384ce3

[willmmiles]

Ideally this would be constexpr too. It's my understanding that best practices for C++ are to return pairs/tuples/structs instead of using reference parameters.

[mathieucarbou]

Done in c384ce3

[willmmiles]

Should we move the implementation to a cpp file? (WebHandlers.cpp maybe)? It's big enough - especially with regex enabled - that it probably won't/oughtn't be inlined anyways, and we can defer to LTO for modern platforms.

[mathieucarbou]

Done in da0fef9.

But on that point, I am not too bothered with that and I like to keep things as much as possible in headers because I prefer a project structurally / cleanly separated like ArduinoJson is done. So keeping things in headers is IMO easier in the future to split concepts in isolated cpp files.

[willmmiles]

Same as matches(), should we move this to a source file?

[mathieucarbou]

Done in da0fef9.

But on that point, I am not too bothered with that and I like to keep things as much as possible in headers because I prefer a project structurally / cleanly separated like ArduinoJson is done. So keeping things in headers is IMO easier in the future to split concepts in isolated cpp files.

[mathieucarbou]

@willmmiles questions addressed ;-)