goldstein.w.n added a comment. In D137181#3935951 <https://reviews.llvm.org/D137181#3935951>, @owenpan wrote:
> In D137181#3935856 <https://reviews.llvm.org/D137181#3935856>, @goldstein.w.n > wrote: > >> In D137181#3935752 <https://reviews.llvm.org/D137181#3935752>, @owenpan >> wrote: >> >>> Please mark review comments as done if you have addressed them. Can you >>> also clean up the test cases, removing overlapping/redundant ones, making >>> sure a test case doesn't end with a newline (e.g., line 5380), etc? >> >> Regarding newlines. I have new line before changing the style. Otherwise >> done. (let me know if you want me to remove those). > > Sorry, it's line 5379 and several other places that have an ending newline. > (Just search for `\n",`.) You only need to remove them in the new test cases. > We can fix the existing ones in a separate NFC patch. Done. Sorry I had thought you mean the newline in the file between the test cases. >> Regarding removing redundant test cases. I think I've had each of them fail >> independently at some point (although many when I was doing the previous >> PPLevel tracking) so I wouldn't call any of them truly redundant. > > I know, but IMO some of the failures (e.g. the `switch` statement) during > development shouldn't automatically go in. This file already has a humongous > size. Maybe do the best you can here. > >> I could remove either the `PPDIS_BeforeHash` or `PPDIS_AfterHash` though as >> the they really no independent logic in this commit. > > Yes, please. Or better yet, alternate a little bit between the two. Done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137181/new/ https://reviews.llvm.org/D137181 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
