Page MenuHomePhabricator

Implement soft reset of the diagnostics engine.
ClosedPublic

Authored by tapaswenipathak on May 22 2022, 11:50 PM.

Details

Summary
Implement soft reset of the diagnostics engine.

This patch implements soft reset and adds tests for soft reset success of the
diagnostics engine.

This patch adds unittest for soft reset success using #define public
private. Multiple options were used before arriving on using #define public
private as discussed in https://github.com/vgvassilev/clang/pull/1.

1. Enable getter for private variables distinguishing soft reset.
2. Using any existing locs
3. gtest:friend_test https://google.github.io/googletest/reference/testing.html#FRIEND_TEST
4. using #define public private accomapnied with #undef private before
and after the inclusion of `.h` file to access a few private members of
enclosing class in the test function.

Finally, a test helper friend function is added to test soft reset
success.

This patch also adds a clang-repl test and adapts clang:tools:clang-repl to
report unsuccess when running with -verify.

Screenshots of testing this patch can be found on the mentioned pull
request.

Co-authored-by: Vassil Vassilev <vvasilev@cern.ch>

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2022, 11:51 PM
tapaswenipathak requested review of this revision.May 22 2022, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2022, 11:51 PM
v.g.vassilev added a subscriber: cfe-commits.
rsmith added a comment.EditedMay 24 2022, 11:00 AM

I don't think we can live with the #define private public approach, not least because this violates the ODR and might lead to compile failures using modules as a result. As an alternative, how about:

  • Adding a friend void DiagnosticTestHelper(); declaration to DiagnosticsEngine
  • Defining that function in DiagnosticTest.cpp and calling it to do your checks of the diagnostics engine's state.

runs clang-format on the diff.

Fixes: https://buildkite.com/llvm-project/premerge-checks/builds/95001#01810b0b-6313-400f-aaf0-35855916ec93

I pasted the wrong diff. sorry!

(have multiple build repository in local)

This comment was removed by tapaswenipathak.

this should be green. :)

v.g.vassilev accepted this revision.Jun 6 2022, 6:32 AM

This looks good to me, modulo the inline comment.

Let's wait for @rsmith's green light.

clang/lib/Interpreter/IncrementalParser.cpp
201
Diags.Reset(/*soft=*/true);
This revision is now accepted and ready to land.Jun 6 2022, 6:32 AM
v.g.vassilev added inline comments.Jun 6 2022, 9:49 AM
clang/lib/Interpreter/IncrementalParser.cpp
200

We should remove this fixme, too.

tapaswenipathak updated this revision to Diff 436750.EditedJun 14 2022, 5:40 AM
tapaswenipathak edited the summary of this revision. (Show Details)

Addresses review comments by @v.g.vassilev.

This revision was automatically updated to reflect the committed changes.

FYI, this change caused warnings when built with GCC:

[1/1] Building CXX object tools/clang/unittests/Basic/CMakeFiles/BasicTests.dir/DiagnosticTest.cpp.o
../tools/clang/unittests/Basic/DiagnosticTest.cpp:17:6: warning: ‘void clang::DiagnosticsTestHelper(clang::DiagnosticsEngine&)’ has not been declared within ‘clang’
   17 | void clang::DiagnosticsTestHelper(DiagnosticsEngine &diag) {
      |      ^~~~~
In file included from ../tools/clang/unittests/Basic/DiagnosticTest.cpp:9:
../tools/clang/include/clang/Basic/Diagnostic.h:548:15: note: only here as a ‘friend’
  548 |   friend void DiagnosticsTestHelper(DiagnosticsEngine &);
      |               ^~~~~~~~~~~~~~~~~~~~~