[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert g_vsc.launch_info to a local variable

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D76593#1936757 <https://reviews.llvm.org/D76593#1936757>, @anton.kolesov wrote:

> In D76593#1936337 <https://reviews.llvm.org/D76593#1936337>, @labath wrote:
>
> > Sounds like a good idea. Could you do the same for `attach_info` (it looks like it should be possible)? Otherwise, one is left wondering about what's the difference...
>
>
> I have that as my next patch - it seems like something that should be in a separate commit, but in the same pull request, but I don't know how to properly submit patch series for review at Phabricator - only through "related revisions", I presume?


You can (and should) use the "related revisions" functionality to denote the relationship between patches, but the purpose of this relationship is to make it easier to review by explaining what depends on what. We don't really have a concept of a "pull request" consisting of multiple patches, which get all committed as a group. Normally each patch should make sense on its own, even if it's not immediately useful (e.g. because it introduces an interface that is not yet used by anyone). The "related" patches can then explain/demonstrate how it's going to be used, but the preceding patches don't need to wait for a green light on the entire group to be committed. If there's some deviation from this (e.g. a preparatory refactoring patch makes some code substantially more complicated and so it may not be desired if the latter patch is not accepted, or there is some uncertainty about the overall direction of the patchset), it should be called out explicitly.

But none of this is really relevant for this patch, which is so simple you could have done it in one commit (and it could be argued that it *should* be done in a single commit, as doing it partway introduces a somewhat inconsistent state, which is undesirable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76593