Skip to content

Store defined symbols #220

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

Merged
merged 3 commits into from
Jun 5, 2019
Merged

Conversation

xbauch
Copy link
Contributor

@xbauch xbauch commented May 10, 2019

during Do_Defining_Identifier, in the symbol table ,if they are not present already.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Does not seem unreasonable. However I would still like to understand why this solves as many bugs as it does. Specifically it seems to resolve a number of other issues for which there are open tickets.

@martin-cs
Copy link
Collaborator

@tjj2017 : your thoughts on this one would be appreciated.

@xbauch xbauch mentioned this pull request May 13, 2019
@@ -1151,6 +1152,20 @@ package body Tree_Walk is
Set_Identifier (Sym, Unique_Name (E));
Set_Type (Sym, Symbol_Type);

if not Global_Symbol_Table.Contains (Sym_Id) then

Choose a reason for hiding this comment

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

Basically, my understanding is this should only happen if we either didn't see the declaration of the type before somehow (which seems unlikely, I hope), or we couldn't parse the declaration (which would explain the many 'fixes' I suppose).

I'm still slightly skeptical this will handle all cases properly, but I'd suspect it's only going to break in cases which are otherwise broken anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to agree with this analysis. I believe it is a general Ada rule that you cannot reference an entity until it has been at least incompletely defined.

@hannes-steffenhagen-diffblue
Copy link
Contributor

It'd still be good to have a test case, though if my suspicion is correct it's actually not possible to construct a stable test case for this...

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

This looks very similar to the work I did on #238. I think in that piece the symbol registering is happening earlier (during the Do_Object_Declaration), because I had also found that some symbols that I was looking up where not in the symtab. Could it be that one of the two works is extraneous or do we need both? I will look into it perhaps a bit later and come back to you on that. Otherwise, LGTM

Copy link
Contributor

@chrisr-diffblue chrisr-diffblue left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@chrisr-diffblue
Copy link
Contributor

chrisr-diffblue commented Jun 3, 2019

I'll rebase this PR as a new PR in #248.

Petr Bauch and others added 3 commits June 5, 2019 07:56
To update the symbol table if the identifier being defined is missing.
1: use wrapper in do_defining identifier
2: avoid duplicate call in do_full_object_declaration
3: update initial value if symbol is present.
@xbauch xbauch force-pushed the feature/store-defined-symbols branch from 86cc890 to 765978d Compare June 5, 2019 07:49
@xbauch xbauch removed the rebase label Jun 5, 2019
@xbauch xbauch merged commit 021a06e into diffblue:master Jun 5, 2019
@xbauch xbauch deleted the feature/store-defined-symbols branch June 10, 2019 12:40
This was referenced Jun 14, 2019
chrisr-diffblue added a commit that referenced this pull request Jun 14, 2019
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.

7 participants