[PATCH] D85585: Replace TableGen range piece punctuator with '..'

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 10 16:36:31 PDT 2020


dblaikie added a comment.

In D85585#2208321 <https://reviews.llvm.org/D85585#2208321>, @Paul-C-Anagnostopoulos wrote:

> In D85585#2208140 <https://reviews.llvm.org/D85585#2208140>, @dblaikie wrote:
>
>> I don't really work with tablegen, so someone else will need to review the substance of this change.
>>
>> One nit (provided inline) and one general question: does the '-' range separator produce any usability problems for users of tablegen, or does it only complicate the implementation? (though, admittedly, {-5--2} isn't terribly readable, so even if it works, I can see usability benefit in changing to {-5..-2})
>
> The primary purpose of the change is to remove a rather odd bit of syntax that requires an almost apologetic description in the documentation.

Seems like the comment would still be needed there, since the syntax rule is still there?

& I'm not sure I understand the text in the spec either - does the (pre-this-patch) first rule never fire, oh, it only fires in examples like the one you've given below "{2-+5}" but in "{2-3}" the second rule fires? Perhaps the first rule should be removed (disallowing "{2-+5}" and instead requiring it be written as "{2-5}") & just leave the second. But it does seem like documenting a weird mostly-implementation detail that still does the right thing so far as the user's concerned? Perhaps the syntax description could be written differently? Guess not if the sign is included in the integer token, then there's no way to separate out just the digit portion and say `digit+ "-" digit+`.

> Negative numbers aren't allowed in ranges, so the worst use of the hyphen would be in something like {2-+5}. But, again, it's mainly to clean up the documentation.

Without removing the old feature, I'm not sure it achieves that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85585/new/

https://reviews.llvm.org/D85585



More information about the llvm-commits mailing list