Skip to content

Commit

Permalink
ClangPlugins: Convert all warnings to errors
Browse files Browse the repository at this point in the history 
Now that the lambda capture plugin isn't full of false-positives, we can
make the jump and start halting builds for these errors. It also allows
these plugins to be useful in CI.
  • Loading branch information
@mattco98
mattco98 committed May 19, 2024
1 parent 407e4ec commit d005c3b
Show file tree
Hide file tree
Showing 9 changed files with 24 additions and 24 deletions.
2 changes: 1 addition & 1 deletion Meta/Lagom/ClangPlugins/LambdaCapturePluginAction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class Consumer
if (capture->capturesThis() || capture->getCaptureKind() != clang::LCK_ByRef)
return;

auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Variable with local storage is captured by reference in a lambda marked ESCAPING");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "Variable with local storage is captured by reference in a lambda marked ESCAPING");
diag_engine.Report(capture->getLocation(), diag_id);

clang::SourceLocation captured_var_location;
Expand Down
10 changes: 5 additions & 5 deletions Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,10 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record)
auto validation_results = validate_field(field);
if (!validation_results.is_valid) {
if (validation_results.is_wrapped_in_gcptr) {
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Specialization type must inherit from JS::Cell");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "Specialization type must inherit from JS::Cell");
diag_engine.Report(field->getLocation(), diag_id);
} else {
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "%0 to JS::Cell type should be wrapped in %1");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "%0 to JS::Cell type should be wrapped in %1");
auto builder = diag_engine.Report(field->getLocation(), diag_id);
if (field->getType()->isReferenceType()) {
builder << "reference"
Expand All @@ -179,7 +179,7 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record)
clang::DeclarationName name = &m_context.Idents.get("visit_edges");
auto const* visit_edges_method = record->lookup(name).find_first<clang::CXXMethodDecl>();
if (!visit_edges_method && !fields_that_need_visiting.empty()) {
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "JS::Cell-inheriting class %0 contains a GC-allocated member %1 but has no visit_edges method");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "JS::Cell-inheriting class %0 contains a GC-allocated member %1 but has no visit_edges method");
auto builder = diag_engine.Report(record->getLocation(), diag_id);
builder << record->getName()
<< fields_that_need_visiting[0];
Expand Down Expand Up @@ -214,7 +214,7 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record)
}

if (!call_to_base_visit_edges_found) {
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Missing call to Base::visit_edges");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "Missing call to Base::visit_edges");
diag_engine.Report(visit_edges_method->getBeginLoc(), diag_id);
}

Expand All @@ -240,7 +240,7 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record)
for (auto const* member_expr : field_access_callback.matches())
fields_that_are_visited.insert(member_expr->getMemberNameInfo().getAsString());

auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "GC-allocated member is not visited in %0::visit_edges");
auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error, "GC-allocated member is not visited in %0::visit_edges");

for (auto const* field : fields_that_need_visiting) {
if (!fields_that_are_visited.contains(field->getNameAsString())) {
Expand Down
2 changes: 1 addition & 1 deletion Tests/ClangPlugins/LambdaTests/lambda_capture_local_by_ref.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ void test()
(void)a;
});

// expected-warning@+1 {{Variable with local storage is captured by reference in a lambda marked ESCAPING}}
// expected-error@+1 {{Variable with local storage is captured by reference in a lambda marked ESCAPING}}
take_fn_escaping([&a] {
(void)a;
});
Expand Down
2 changes: 1 addition & 1 deletion Tests/ClangPlugins/LibJSGCTests/calling_base_visit_edges_not_through_using_decl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
class TestClass : public JS::Object {
JS_OBJECT(TestClass, JS::Object);

// expected-warning@+1 {{Missing call to Base::visit_edges}}
// expected-error@+1 {{Missing call to Base::visit_edges}}
virtual void visit_edges(Visitor& visitor) override
{
JS::Object::visit_edges(visitor);
Expand Down
20 changes: 10 additions & 10 deletions Tests/ClangPlugins/LibJSGCTests/cell_member_not_wrapped.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ class TestClass : public JS::Object {
visitor.visit(m_object_ptr);
}

// expected-warning@+1 {{reference to JS::Cell type should be wrapped in JS::NonnullGCPtr}}
// expected-error@+1 {{reference to JS::Cell type should be wrapped in JS::NonnullGCPtr}}
JS::Object& m_object_ref;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
JS::Object* m_object_ptr;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
Vector<JS::Object*> m_objects;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
NewType1* m_newtype_1;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
NewType2* m_newtype_2;
};

Expand All @@ -50,14 +50,14 @@ class TestClassNonCell {
}

private:
// expected-warning@+1 {{reference to JS::Cell type should be wrapped in JS::NonnullGCPtr}}
// expected-error@+1 {{reference to JS::Cell type should be wrapped in JS::NonnullGCPtr}}
JS::Object& m_object_ref;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
JS::Object* m_object_ptr;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
Vector<JS::Object*> m_objects;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
NewType1* m_newtype_1;
// expected-warning@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
// expected-error@+1 {{pointer to JS::Cell type should be wrapped in JS::GCPtr}}
NewType2* m_newtype_2;
};
2 changes: 1 addition & 1 deletion Tests/ClangPlugins/LibJSGCTests/missing_call_to_base_visit_edges.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
class TestClass : public JS::Object {
JS_OBJECT(TestClass, JS::Object);

// expected-warning@+1 {{Missing call to Base::visit_edges}}
// expected-error@+1 {{Missing call to Base::visit_edges}}
virtual void visit_edges(Visitor&) override
{
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/ClangPlugins/LibJSGCTests/missing_member_in_visit_edges.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ class TestClass : public JS::Object {
Base::visit_edges(visitor);
}

// expected-warning@+1 {{GC-allocated member is not visited in TestClass::visit_edges}}
// expected-error@+1 {{GC-allocated member is not visited in TestClass::visit_edges}}
JS::GCPtr<JS::Object> m_object;
};
2 changes: 1 addition & 1 deletion Tests/ClangPlugins/LibJSGCTests/missing_visit_edges_method.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

#include <LibJS/Runtime/Object.h>

// expected-warning@+1 {{JS::Cell-inheriting class TestClass contains a GC-allocated member 'm_cell' but has no visit_edges method}}
// expected-error@+1 {{JS::Cell-inheriting class TestClass contains a GC-allocated member 'm_cell' but has no visit_edges method}}
class TestClass : public JS::Object {
JS_OBJECT(TestClass, JS::Object);

Expand Down
6 changes: 3 additions & 3 deletions Tests/ClangPlugins/LibJSGCTests/wrapping_non_cell_member.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
struct NotACell { };

class TestClass {
// expected-warning@+1 {{Specialization type must inherit from JS::Cell}}
// expected-error@+1 {{Specialization type must inherit from JS::Cell}}
JS::GCPtr<NotACell> m_member_1;
// expected-warning@+1 {{Specialization type must inherit from JS::Cell}}
// expected-error@+1 {{Specialization type must inherit from JS::Cell}}
JS::NonnullGCPtr<NotACell> m_member_2;
// expected-warning@+1 {{Specialization type must inherit from JS::Cell}}
// expected-error@+1 {{Specialization type must inherit from JS::Cell}}
JS::RawGCPtr<NotACell> m_member_3;
};

0 comments on commit d005c3b

Please sign in to comment.