Add DiagnosticsEngineRAII by codegallivant · Pull Request #849 · compiler-research/CppInterOp

codegallivant · 2026-03-10T17:50:03Z

Description

Fixes FIXME: this can go into a RAII object. Reference:

CppInterOp/lib/CppInterOp/CppInterOp.cpp

Line 311 in 852f6d4

// FIXME: this can go into a RAII object

Implementation details-
I added a RAII class (compat::DiagnosticsEngineRAII) that automatically checks if it should reset the DiagnosticsEngine before the RAII object goes out of scope. It checks for diags.hasErrorOccurred() and a user-provided boolean reset_condition.

Type of change

Please tick all options which are relevant.

Bug fix
New feature
Requires documentation updates

Testing

100% tests passed, 0 tests failed out of 3

Label Time Summary:
DEPENDS    =  12.35 sec*proc (3 tests)

Total Test time (real) =  12.36 sec
[100%] Built target check-cppinterop

Checklist

I have read the contribution guide recently

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-10T18:04:13Z

  // ~SynthesizingCodeRAII() {} // TODO: implement
 };
+
+class DiagnosticsEngineRAII {


warning: class 'DiagnosticsEngineRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class DiagnosticsEngineRAII { ^

github-actions · 2026-03-10T18:04:13Z

+
+class DiagnosticsEngineRAII {
+private:
+  clang::DiagnosticsEngine& diags;


warning: member 'diags' of type 'clang::DiagnosticsEngine &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

clang::DiagnosticsEngine& diags; ^

github-actions · 2026-03-10T18:04:13Z

+  clang::DiagnosticsEngine& diags;
+
+public:
+  bool reset_condition; // additional condition to reset diagnostics


warning: member variable 'reset_condition' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

bool reset_condition; // additional condition to reset diagnostics ^

codecov · 2026-03-10T18:07:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.74%. Comparing base (4e3e4c2) to head (fec15ed).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
+ Coverage   79.72%   79.74%   +0.02%     
==========================================
  Files          11       11              
  Lines        4025     4029       +4     
==========================================
+ Hits         3209     3213       +4     
  Misses        816      816

Files with missing lines	Coverage Δ
lib/CppInterOp/Compatibility.h	`87.60% <100.00%> (+0.64%)`	⬆️
lib/CppInterOp/CppInterOp.cpp	`87.94% <100.00%> (-0.02%)`	⬇️

Files with missing lines	Coverage Δ
lib/CppInterOp/Compatibility.h	`87.60% <100.00%> (+0.64%)`	⬆️
lib/CppInterOp/CppInterOp.cpp	`87.94% <100.00%> (-0.02%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Vipul-Cariappa

Can you check if clang's SFINAETrap (Ref: https://clang.llvm.org/doxygen/classclang_1_1Sema_1_1SFINAETrap.html#details) does the necessary trick? i.e. can we replace our custom logic with this object instead?

codegallivant · 2026-03-11T17:20:59Z

Can you check if clang's SFINAETrap (Ref: https://clang.llvm.org/doxygen/classclang_1_1Sema_1_1SFINAETrap.html#details) does the necessary trick? i.e. can we replace our custom logic with this object instead?

@Vipul-Cariappa I spent some time reading up on a few things. From what I've found, this is my understanding:

SFINAETrap() only suppresses errors that are not outside the immediate context of substitution i.e. not outside the function's signature. SFINAETrap() is supposed to be used to parse template/overloaded function signatures without causing false errors. It is internally hard-coded to not suppress errors resulting from the function's body itself.

Refer to the following snippet from CppImpl::InstantiateFunctionDefinition -

getSema().InstantiateFunctionDefinition(SourceLocation(), FD,
                                            /*Recursive=*/true,
                                            /*DefinitionRequired=*/true // look at this argument

and I found this in sema.h-

  /// \param DefinitionRequired if true, then we are performing an explicit
  /// instantiation where the body of the function is required. Complain if
  /// there is no such body.

The DefinitionRequired=true argument asks clang to mandatorily parse the function's body and add it to the AST. So, an error that results from this will be a hard error and would not get suppressed by SFINAETrap(). Therefore, I do not think using SFINAETrap() here would be useful.

Sources:

https://clang.llvm.org/doxygen/classclang_1_1Sema_1_1SFINAETrap.html#details - within the function description, it says "from immediate context".
https://discourse.llvm.org/t/class-template-methods/36237/15 - Someone tried something very similar to what we're trying to do but ran into the same issue with SFINAETrap(). In the last comment, the maintainer tells him that SFINAETrap() only suppresses errors "from immediate context of substitution" and said "We don't have a way to suppress those diagnostics and return to a state as if the instantiation never happened", unless you use a "flag on the DiagnosticsEngine to suppress all diagnostics".

codegallivant · 2026-03-12T22:47:03Z

cling builds and git-clang-format precheck are failing. will push fixes soon.

edit: done. all checks passed.

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-12T23:25:14Z

 #endif
 }
+
+class DiagnosticsEngineRAII {


warning: class 'DiagnosticsEngineRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class DiagnosticsEngineRAII { ^

github-actions · 2026-03-12T23:25:14Z

+
+class DiagnosticsEngineRAII {
+private:
+  clang::DiagnosticsEngine& diags;


warning: member 'diags' of type 'clang::DiagnosticsEngine &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

clang::DiagnosticsEngine& diags; ^

github-actions · 2026-03-12T23:25:14Z

+  clang::DiagnosticsEngine& diags;
+
+public:
+  bool reset_condition; // additional condition to reset diagnostics


warning: member variable 'reset_condition' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

bool reset_condition; // additional condition to reset diagnostics ^

codegallivant · 2026-03-16T03:53:22Z

@Vipul-Cariappa hello, would appreciate feedback about this, thanks.

Vipul-Cariappa

The way this is implemented, it looks more like a function call instead of a RAII object (Look at the way reset_condition is used). But I don't know if a simple function call is better or a RAII object is better...

vgvassilev · 2026-03-21T16:24:06Z

The way this is implemented, it looks more like a function call instead of a RAII object (Look at the way reset_condition is used). But I don't know if a simple function call is better or a RAII object is better...

Wasn't the point here to use clang::Sema::SFINAETrap?

codegallivant · 2026-03-21T16:28:23Z

Wasn't the point here to use clang::Sema::SFINAETrap?

Please refer to my analysis in this comment: #849 (comment)
I may be wrong, am new to this. Would appreciate feedback.

vgvassilev · 2026-03-23T16:26:51Z

Wasn't the point here to use clang::Sema::SFINAETrap?

Please refer to my analysis in this comment: #849 (comment) I may be wrong, am new to this. Would appreciate feedback.

Did you try to implement it and how does it fail?

github-actions Bot reviewed Mar 10, 2026

View reviewed changes

codegallivant force-pushed the diags-raii branch from 120803e to 96d3edd Compare March 10, 2026 21:35

Vipul-Cariappa reviewed Mar 11, 2026

View reviewed changes

codegallivant force-pushed the diags-raii branch from 96d3edd to fadf560 Compare March 12, 2026 16:41

codegallivant force-pushed the diags-raii branch 2 times, most recently from c2897ef to 72eed5a Compare March 12, 2026 23:16

github-actions Bot reviewed Mar 12, 2026

View reviewed changes

add DiagnosticsEngineRAII

fec15ed

codegallivant force-pushed the diags-raii branch from 72eed5a to fec15ed Compare March 13, 2026 00:12

codegallivant requested a review from Vipul-Cariappa March 13, 2026 01:58

Vipul-Cariappa reviewed Mar 20, 2026

View reviewed changes

Conversation

codegallivant commented Mar 10, 2026

Description

Type of change

Testing

Checklist

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

codegallivant commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codegallivant commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

codegallivant commented Mar 16, 2026

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented Mar 21, 2026

Uh oh!

codegallivant commented Mar 21, 2026

Uh oh!

vgvassilev commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Mar 10, 2026 •

edited

Loading

codegallivant commented Mar 11, 2026 •

edited

Loading

codegallivant commented Mar 12, 2026 •

edited

Loading