[PATCH] D24686: [support] Some improvements to StringSwitch

[Zachary Turner via llvm-commits]

Yea I don't like the stateful modifiers.  It reminds me of std::iostream
where you had to do << std::hex(foo) << 12345 << std::dec(foo).  I have an
idea in my head for how you could reduce everything to one function, but
it's a lot of work.  I could imagine something like this though:

llvm::StringSwitch<int>(S).Case(Lower("int", "double", "foo"), Value(7)).

If you do it like this, Lower could be variadic, and it would remove all
the overloads and code duplication and you could just have 3 or 4 functions
in the class.  Case, StartsWith, EndsWith, and maybe Contains.  Perhaps you
could even overload the operator() so you could write it like this:

llvm::StringSwitch<int>(S)
    (CaseLower("int", "double", "foo"), Value(7))
    (Case("ABC", "DEF", "GHI"), Value(42))
    (StartsWith("Test", "TeSt"), Value(84));

Anyway, it's a lot of work, and for now I don't think we should lose the
ability to have Case and CaseLower on the same switch.

On Mon, Oct 3, 2016 at 1:35 PM Rui Ueyama <ruiu at google.com> wrote:

> Well, I don't have data points to make a decision, so if you like that way
> I don't object. The other way would be to make it switchable, e.g.
>
>   auto Ret = StringSwtich<T>(Str)
>     .Case("case-sensitive", foo)
>     .CaseInsensitive()
>     .Case("case-insensitive", bar)
>     .CaseSensitive()
>     ...
>
> but it's probably too much.
>
> On Mon, Oct 3, 2016 at 1:30 PM, Zachary Turner <zturner at google.com> wrote:
>
> That is true, but it would make it impossible to have both case sensitive
> and case-insensitive cases in the same switch.  For example, the following
> is currently possible:
>
> StringSwitch<int>(s).Case("one").CaseLower("two");
>
> I'm not sure how useful this would be, but I see no reason not to allow
> it.  One idea might be to templatize the member function itself, and give
> it a default value of case sensitive.  But then you still have to write:
>
> StringSwitch<int>(s).Case<>("one").Case<false>("two");
>
> which might be awkward.  thoughts?
>
> On Mon, Oct 3, 2016 at 1:26 PM Rui Ueyama <ruiu at google.com> wrote:
>
> It just comes to my mind that you could make StringSwitch case-insensitive
> instead of adding case-insensitive versions of StartsWith, Case and Cases.
> It'd probably be something like
>
>   auto Ret = StringSwtich<T>(Str, /*CaseInsensitive=*/true)
>       .Case("foo", bar) //  "foo" is compared in the insensitive manner
>       ...
>
> With this, you don't need to double the number of member functions.
>
> On Mon, Oct 3, 2016 at 1:15 PM, Zachary Turner <zturner at google.com> wrote:
>
> zturner updated this revision to Diff 73327.
> zturner added a comment.
>
> Unit tests for the case-sensitive versions were removed and submitted
> separately.  The `Optional` variant of the function is removed and will be
> submitted later.  This patch only contains case-insensitive versions of the
> functions with associated unit tests.
>
>
> https://reviews.llvm.org/D24686
>
> Files:
>   include/llvm/ADT/StringSwitch.h
>   unittests/ADT/StringSwitchTest.cpp
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161003/1e1fd2c8/attachment.html>