Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Implement soft reset of the diagnostics engine.

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


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

1. Enable getter for private variables distinguishing soft reset.
2. Using any existing locs
3. gtest: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

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

Co-authored-by: Vassil Vassilev <>

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.


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.

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

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 &);
      |               ^~~~~~~~~~~~~~~~~~~~~