[PATCH] D83592: [Coverage] Add comment to skipped regions

This revision was automatically updated to reflect the committed changes. Closed by commit rGb46176bbb094: Reland [Coverage] Add comment to skipped regions (authored by zequanwu). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. @zequanwu the change that dropped merging of adjacent comments looks good. If no further issues cropped up during stage2 testing, please feel free to re-land. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. @vsk Do you think it's ok to reland this commit? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. In D83592#2174600 , @vsk wrote: > In D83592#2170912 , @zequanwu wrote: > > > I don't why when using the cmake option > > `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On` to test coverage for clang

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. In D83592#2170912 , @zequanwu wrote: > I don't why when using the cmake option > `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On` to test coverage for clang itself, it > does tracking comments. > Also, with that option on, `llvm-cov`

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) zequanwu wrote: > hans wrote: > > zequanwu wrote: > > > @hans , can you

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 280504. zequanwu added a comment. Change `newSR.ColumnEnd = 1` to `newSR.ColumnEnd = SR.ColumnStart + 1` to ensure ColumnEnd >= ColumnStart. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files:

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) hans wrote: > zequanwu

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) zequanwu wrote: > @hans , can you take a look on this part? I saw

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) @hans , can you take a

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 280283. zequanwu edited the summary of this revision. zequanwu added a comment. In `Preprocessor.cpp`, don't increment `TokenCount` if `LexLevel == 0`. I tests this with chromium crypto_unittests. It doesn't track comments regions. I don't why when using

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. Just tried the cmake option `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On`, the result still shows count for comments... but it pass the `coverage_comments.cpp` test case, need more investigation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 280005. zequanwu added a comment. Fix the bug about merging skipped regions. Simple don't merge, because `adjustSkippedRange` will handle those cases if `NextTokLoc` is invalid, which will just skip the next if condition. CHANGES SINCE LAST ACTION

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. I found the problem is that I was trying to merge two skipped regions when they are adjacent. So, the following code will fail at assertion, because the skipped regions are in different files, but are merged. $ cat a.h // comment $ cat a.c #include "a.h" //

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. In D83592#2167926 , @vsk wrote: > @zequanwu I'm not sure whether this is something you've already tried, but > for frontend changes, it can be helpful to verify that a stage2 clang > self-host with assertions enabled completes

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk added a comment. @zequanwu I'm not sure whether this is something you've already tried, but for frontend changes, it can be helpful to verify that a stage2 clang self-host with assertions enabled completes successfully. For coverage specifically, it's possible to do that by setting

[PATCH] D83592: [Coverage] Add comment to skipped regions

MaskRay added a comment. (This was committed within 10 minutes after it was approved. As a large change with updates to some sensitive places like clang/Lex/Preprocessor.h, this probably should have been waited a bit longer.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added a comment. This caused asserts in Chromium's coverage builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1108352 I've reverted it in the meantime (238bbd48c5a5f84deca36e5df980241578f7c1df ). It also

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu added a comment. In D83592#2165818 , @thakis wrote: > Looks like this breaks tests on Windows: > http://45.33.8.238/win/20315/step_7.txt > > Please take a look, and if it takes a while to investigate please revert > while you look. Fixed

[PATCH] D83592: [Coverage] Add comment to skipped regions

thakis added a comment. Looks like this breaks tests on Windows: http://45.33.8.238/win/20315/step_7.txt Please take a look, and if it takes a while to investigate please revert while you look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: compiler-rt/test/profile/coverage_comments.cpp:9 +int y = 0; /* comment */ // CHECK: [[# @LINE]]| 1| int y = 0; /* comment */ +int z = 0; // comment // CHECK:

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu closed this revision. zequanwu added a comment. Landed here, https://reviews.llvm.org/rGabd45154bdb6b76c5b480455eacc8c75b08242aa I put the wrong diff ID.. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

[PATCH] D83592: [Coverage] Add comment to skipped regions

MaskRay added inline comments. Comment at: compiler-rt/test/profile/coverage_comments.cpp:9 +int y = 0; /* comment */ // CHECK: [[# @LINE]]| 1| int y = 0; /* comment */ +int z = 0; // comment // CHECK: [[# @LINE]]| 1| int z = 0; // comment +

[PATCH] D83592: [Coverage] Add comment to skipped regions

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 ___ cfe-commits mailing list

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 279661. zequanwu added a comment. remove unnecessary header. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: clang/include/clang/Lex/Preprocessor.h clang/lib/CodeGen/CodeGenAction.cpp

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 279605. zequanwu added a comment. Change `@LINE` to `# @LINE`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: clang/include/clang/Lex/Preprocessor.h clang/lib/CodeGen/CodeGenAction.cpp

[PATCH] D83592: [Coverage] Add comment to skipped regions

MaskRay added a comment. New tests should use `[[#@LINE]]` instead of `[[@LINE]]` https://llvm.org/docs/CommandGuide/FileCheck.html > To support legacy uses of @LINE as a special string variable, FileCheck also > accepts the following uses of @LINE with string substitution block syntax: >

[PATCH] D83592: [Coverage] Add comment to skipped regions

zequanwu updated this revision to Diff 279595. zequanwu marked 2 inline comments as done. zequanwu retitled this revision from "[Parser] Add comment to skipped regions" to "[Coverage] Add comment to skipped regions". zequanwu edited the summary of this revision. zequanwu added a comment. -