Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recursive structs in templates #1782 #1783

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

intrigus-lgtm
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm commented Apr 14, 2024

TODO: Add explanation.

@intrigus-lgtm intrigus-lgtm force-pushed the fix-recursive-structs-in-templates branch 2 times, most recently from 83a66b1 to 439875a Compare April 14, 2024 20:46
Copy link
Member

@mikhailramalho mikhailramalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like you need to add the extra argument in the c and cpp frontend. Also always add an explanation of your PR

gh-1782.cc

^ERROR: main symbol `main' not found
^ERROR: CONVERSION ERROR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add an empty main here and make the test pass.

@@ -359,6 +366,10 @@ bool clang_c_convertert::get_struct_union_class(const clang::RecordDecl &rd)
* or it's already a complete struct or union in the context. */
if (!sym->type.incomplete())
return false;

/* If we're only forward declaring, we don't need to complete the type */
if (only_forward_declare)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just call isThisDeclarationADefinition() here?

@@ -1210,7 +1228,8 @@ template <typename SpecializationDecl>
bool clang_cpp_convertert::get_template_decl_specialization(
const SpecializationDecl *D,
bool DumpExplicitInst,
exprt &new_expr)
exprt &new_expr,
bool OnlyForwardDeclare)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe check D->isThisDeclarationADefinition() here?

@intrigus-lgtm intrigus-lgtm force-pushed the fix-recursive-structs-in-templates branch from 439875a to 5166f78 Compare April 15, 2024 22:58
@intrigus-lgtm
Copy link
Contributor Author

It doesn't look like you need to add the extra argument in the c and cpp frontend. Also always add an explanation of your PR

Apologies, I should have opened this as a draft instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants