HazardyKnusperkeks wrote:

> @rymiel @HazardyKnusperkeks Thank you for your review! I have fixed the 
> points. But for refactoring of the test base class in 
> [f8d10d5](https://github.com/llvm/llvm-project/commit/f8d10d5ac9ab4b45b388c74357fc82fb96562e66)
>  . I'm not sure I should do here, and if I should, I should do it in splitted 
> pull request.
> 
> Now I really understand I should split this pull request into some parts. At 
> first it is large and continue growing by adding documents. I'm wondering how 
> and current plan is separating semantically,
> 
>     * Handling multi line string (~100 lines).
> 
>     * Handling numeric like identifier (~100 lines).
> 
>     * Handling TableGen specific keywords (~100 lines)
> 
>     * Unwrapped line parsing(~100 lines).
> 
>     * Parse TableGen values (about 500+ lines including unittest).
> 
>     * Basic options (but for aligning ones) (about 500+ lines including the 
> document).
> 
>     * Aligning options (about 100 lines including document).
> 
>     * Refactor unittests.
> 
> 
> I'm not sure this is good plan. They may be complicated. Could you help me to 
> plan if you have some idea?
> 
> In addition, I do not know the appropriate way to split pull request after I 
> made one. Is it enough to refer each other, and abort this at last?
Just create a pull request per feature/bug. You can sill use this for one of 
them, or abandon this. If some of them build on each other there is, as far as 
I know, no good way. 

The few times I came to this I created multiple PRs and the top most commit(s) 
is/are part of multiple PRs and I mentioned this. Then we have to make sure 
that they are merged correctly. With multiple local branches you can still work 
on all of them in parallel.

My experience (here and at work) is, that smaller changes are easier to check 
and you get a faster response. Because 50~150 lines changes I can just review 
and move on. A 500+ change review will more likely be postponed.

https://github.com/llvm/llvm-project/pull/76059
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to