skan marked an inline comment as done. skan added inline comments.
================ Comment at: lib/Frontend/HeaderIncludeGen.cpp:55 + // Simplify Filename that starts with "./" + if (Filename.startswith("./")); + Filename=Filename.substr(2); ---------------- lebedev.ri wrote: > skan wrote: > > lebedev.ri wrote: > > > skan wrote: > > > > craig.topper wrote: > > > > > skan wrote: > > > > > > lebedev.ri wrote: > > > > > > > xiangzhangllvm wrote: > > > > > > > > Need remove ";" ? > > > > > > > This was fixed but no test was added? > > > > > > > Was some existing test failing previously? Which one? > > > > > > The test is in the file 'clang_H_opt.c' which is included in this > > > > > > patch. > > > > > The extra semicolon would have caused the body of the 'if' to execute > > > > > unconditionally. Did any existing test case fail for that in your > > > > > local testing? Or did you not test with that mistake? > > > > i fixed the mistake in the updated patch. I ran the test in > > > > 'clang_H_opt.c' alone for this patch. The extra semicolon caused the > > > > body of `if` to exeute always, which didn't cause the test to fail. > > > > The extra semicolon caused the body of if to exeute always, which > > > > didn't cause the test to fail. > > > > > > That is the question. > > > Is there test coverage with that error? > > No. I just run alll the tests for clang, and all of them can pass even if > > the extra semicolon exits. If we want to add a test to cover that error, > > we have to add a headerfile outside the 'test/Driver' directory. Do we need > > to add the test to cover that error? > Then yes please, do add test coverage for that bug :) test for that bug has been added in the new patch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62115/new/ https://reviews.llvm.org/D62115 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits