[PATCH] D77605: [FileCheck] Fix --dump-input implicit pattern location

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

jdenny marked an inline comment as done.
jdenny added a comment.

In D77605#1970617 <https://reviews.llvm.org/D77605#1970617>, @thopre wrote:

> In D77605#1970014 <https://reviews.llvm.org/D77605#1970014>, @jdenny wrote:
>
> > I'm new to working with Harbormaster.  Do I manually force it to re-run after updating a patch?
>
>
> I've seen it do it automatically in other phabricator instances but that doesn't seem to be the case here. Might be configuration-dependent. Better force a rebuild to be sure.


Thanks.  I clicked restart.  We'll see what happens.



================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:320-322
+      // The implicit buffer ID is the buffer ID minus the input file buffer
+      // and the check file buffer plus one for one-origin indexing.
+      Label << "imp" << (CheckBufferID - 1);
----------------
thopre wrote:
> Is this going to be stable if a new option that require a buffer gets added? I know the tests would catch this if that's the case but I'd prefer to not have to update this formula.
> 
> I'm also confused at the comment. Does it match the formula? All I see in the formula is that the implicit buffer ID is the check file buffer minus one. Does the comment need updating? Actually repeating the formula does not seem useful, I'd rather have an explanation as to why the implicit buffer ID can be obtained with this formula.
> Is this going to be stable if a new option that require a buffer gets added? I know the tests would catch this if that's the case but I'd prefer to not have to update this formula.

Good point.  I suppose I could pass around the buffer ID range for implicit buffers.

Of course, if new buffers are added for patterns, this code likely has to be updated to process annotations for those new buffers anyway.  But your suggestion would at least address the case where a new buffer is added for some other purpose.

> I'm also confused at the comment. Does it match the formula?

It does.

> All I see in the formula is that the implicit buffer ID is the check file buffer

It's not the `CheckFileBufferID`, it's the `CheckBufferID`.  Perhaps I should rename the latter to `BufferID` so it's more distinct?

> minus one.

I could rewrite it as `- 2 + 1`.  That is, `- 2` is for the input file buffer and check file buffer, and `+ 1` converts from zero-origin to one-origin indexing.  Would that help?

Well, it's moot if I pass in the implicit buffer ID range.


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

https://reviews.llvm.org/D77605