[PATCH] D82203: [FileCheck] Implement -dump-input-context and -dump-input-filter

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D82203#2118886 <https://reviews.llvm.org/D82203#2118886>, @lattner wrote:

> Ok, but instead of allowing "fine grained control", why not allow a "tasteful default" and "all" option?  The "all" option seems perfectly reasonable for the weird case,


Hopefully we can reach a consensus on what's tasteful.  I figured there would always be someone unhappy with the default, but then they'd have a way out via these options and `FILECHECK_OPTS`.

> and eliminates a big configuration matrix.  This would just entail turning the command line option into a magic number / enum.

If the configuration space would reduce to a bool that either selects the default or all, there would be two main changes in terms of implementation and testing:

1. The elimination of `-dump-input-context` would mean that `DumpInputContext` would be replaced by 5 (or whatever value people ultimately agree on).  That doesn't seem like much of a win.  I will also need to rewrite the test `dump-input-context.txt`, which checks that the right amount of context is shown vs. elided in various boundary cases.  It currently tests this by varying `-dump-input-context`, but I'll need to change it to vary the amount of input instead.  At first glance, that seems more complex to write and maintain than what's there now, especially if the magic 5 should ever change, but maybe I'm overlooking a trick.

2. The change to `-dump-input-filter` would mean that `FindInputLineInFilter` would handle only two cases instead of four.  However, the infrastructure to support those two cases is the same infrastructure needed to support all four.  The result is that only this one already simple function would shrink.  Again, that doesn't seem like a win.  The test `dump-input-filter.txt` will also get a little smaller, but not much less complex, in my opinion.

If the win is that the configuration space would be significantly easier to understand for a user, I might be able to buy that for #2 but not for #1.

A middle ground here might be to drop `-dump-input-filter` but keep `-dump-input-context` as is.  People can specify `-dump-input-context=99999` (or maybe some magic string like `all`) if they want all the context.

What do you think?  Thanks for your feedback.


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

https://reviews.llvm.org/D82203