Tobias, Thanks very much for the review.
On Thu, Apr 9, 2020 at 5:21 AM Tobias Burnus <tob...@codesourcery.com> wrote: > > Hi, > > On 4/6/20 7:25 PM, Fritz Reese via Fortran wrote: > > > The attached patch fixes PR 87923 while also simplifying the code in > > io.c. > > Thanks for the work, which looks great; it is a bit lengthy > but mostly moving code or mechanical (%C → %L). > It also has an impressive amount of new test cases! I also wished the patch could be easier on the eyes, but alas sometimes this is the price of progress. :-) > * First line in git commit "Fix fortran/87923 -- ICE(s) when resolving I/O > tags." > It helps with doing patch archeology if they are the same – or if the GIT > one > is a substring of the email subject. (If it is about a PR, searching for > the PR > usually works, but also not al emails have the PR number in the subject.) > For this patch, you use: > email: "[PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O > tags and simplify io.c" > GIT: "Fix fortran/87923 -- ICE(s) when resolving I/O tags." Yes, that is a good point. I will alter the commit summary to match the email subject. > * Please check whether the ChangeLog lines are too long; I didn't count > but it looks as if they might be too long. For sure, they > were too long for your mail program … I copied the git commit message from the log, which git formats with an extra level of indentation. I wrapped the raw ChangeLog entries and commit message to 80 characters, but after the extra indentation my mail client indeed wrapped the lines during post-processing. I suppose I should wrap these each to 76 to account for git's indentation. That certainly makes "git log" look better. > * In the following comment, you have two empty tailing lines: > > + Tag expressions are already resolved by resolve_tag, which includes > + verifying the type, that they are scalar, and verifying that BT_CHARACTER > + tags are of default kind. > + > + */ Oops! I will commit the patch with these fixes rebased on master after one final build+test. Thanks again for taking a look. Cheers, --- Fritz Reese
