dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed.
I'm not sure D104465 <https://reviews.llvm.org/D104465> (the motivation for this patch) is the right approach, but we can have that discussion over there. ================ Comment at: clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp:24 + SourceLocation(), + /*PadOutputToInputSize=*/true); +} ---------------- Why turn this on all the time? That adds a lot of noise below. Can you instead just turn it on for specific targeted tests? Also, maybe I just missed it, but I don't see any tests that check the behaviour of this flag. ================ Comment at: clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp:113-115 ASSERT_FALSE( minimizeSourceToDependencyDirectives("#define MACRO\n\n\n", Out)); + EXPECT_EQ("#define MACRO", rtrim(Out)); ---------------- I'm uncomfortable dropping the check that `#define MACRO\n\n\n` turns into precisely `#define MACRO\n`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104462/new/ https://reviews.llvm.org/D104462 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
