gematria icon indicating copy to clipboard operation
gematria copied to clipboard

Add test for `BasicBlockGraphBuilder::DebugString`.

Open virajbshah opened this issue 6 months ago • 5 comments

  • Add a test to make sure DebugString returns something reasonable.

virajbshah avatar Aug 07 '25 13:08 virajbshah

Is this supposed to be stacked on the PR with the fix?

boomanaiden154 avatar Aug 07 '25 13:08 boomanaiden154

Yes, the expected output matches the fixed list string representation.

virajbshah avatar Aug 07 '25 13:08 virajbshah

Yes, the expected output matches the fixed list string representation.

This includes the change in the PR that does the fix. I was mainly asking to confirm that you plan on landing the lower one first.

If you use a tool like spr, the relationships become more explicit.

boomanaiden154 avatar Aug 07 '25 20:08 boomanaiden154

This includes the change in the PR that does the fix. I was mainly asking to confirm that you plan on landing the lower one first.

Yeah, ideally I'd have this PR have its base point to the branch corresponding to the PR with the StrAppendList fix to keep commits from there out of this PR (but still build on top of them), but since both branches are on my fork, GitHub would open a PR on my fork instead of here, so I'd have to have this PR compare against main (and so include commits from previous unmerged PRs).

If you use a tool like spr, the relationships become more explicit.

I'm currently using jj, which makes the stacked workflow a lot more convenient locally, but it looks like something like spr still needs to complement it for the GitHub side of things (which sounds like it should really just be making all PRs base off of the right branch and merge them in the right order).

virajbshah avatar Aug 07 '25 21:08 virajbshah

Yeah, ideally I'd have this PR have its base point to the branch corresponding to the PR with the StrAppendList fix to keep commits from there out of this PR (but still build on top of them), but since both branches are on my fork, GitHub would open a PR on my fork instead of here, so I'd have to have this PR compare against main (and so include commits from previous unmerged PRs).

You can fix that by creating branches in the main repository (ie google/gematria), not your fork.

I'm currently using jj, which makes the stacked workflow a lot more convenient locally, but it looks like something like spr still needs to complement it for the GitHub side of things (which sounds like it should really just be making all PRs base off of the right branch and merge them in the right order).

You need a tool that explicitly has support for Github's way of doing things. spr is the only one I know of. We can't use Graphite because this is an LLVM repository.

boomanaiden154 avatar Aug 08 '25 13:08 boomanaiden154