Skip to content

Do cu specs #113

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 5 commits into from
Jan 28, 2019
Merged

Do cu specs #113

merged 5 commits into from
Jan 28, 2019

Conversation

xbauch
Copy link
Contributor

@xbauch xbauch commented Jan 22, 2019

Mostly Trevor's work, the last commit addresses some issues from xbauch#2 and adds a test for packages.

Unit_Name = "system%s"
then
null;
-- At the moment Standard or System are not processed - to be done
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some Put_Line warning about this?
In the message not sure if 'package' or 'unit' might be more appropriate
Put_Line (Standard_Error,
"At the moment Standard or System library units are not processed. ");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take it back: not processing standard/system libraries is not a problem unless you rely on something in them (and then you would get the appropriate error message).

Copy link
Contributor

Choose a reason for hiding this comment

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

@xbauch Nitpick:
Okay. The "if then else" construct begs the question.
To prevent another reader wondering why the null branch of the "if then else" rather than just just "if not" maybe add a clarifying line?
I'll assume the if else then construct is in case in the future we want to consider doing something with standard/system libraries.

case Nkind (N) is
when N_Subprogram_Body =>
if Acts_As_Spec (N) then
-- The unit is a withed library unit which subprogram body
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest add summary line at top (similar to this)
-- withed library unit that has no separate declaration
(this gives the reader what is common among the scenario discussed below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@xbauch xbauch force-pushed the do_cu_specs branch 2 times, most recently from 6184ba2 to a8b77a2 Compare January 24, 2019 14:53
@xbauch
Copy link
Contributor Author

xbauch commented Jan 24, 2019

@sonodtt and @hannes-steffenhagen-diffblue I've addressed the comments made here and on xbauch#2. I have added commit messages summarising their content.

Copy link
Collaborator

@tjj2017 tjj2017 left a comment

Choose a reason for hiding this comment

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

I am happy if you change the "-- Withed library ..." comment and we can delay the renaming of procedures until my next PR

else

-- Withed library unit that has no separate declaration
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand this comment. These withed library units do have library-level declarations. Any user defined library-level unit must have an explicit declaration whether it is a package, subprogram declaration or just a subprogram body. This is what I tried to explain in the comments under each case.

A more general comment preceding the case statement might be
"-- Handle all other withed library unit declarations"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, changed.

@@ -204,6 +207,12 @@ package body Tree_Walk is
function Do_Op_Not (N : Node_Id) return Irep
with Pre => Nkind (N) in N_Op;

procedure Do_Package_Declaration (N : Node_Id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I seemed to have introduced a number of procedures starting with Do_* which is not in accordance with the naming convention according to xbauch#2 (comment).

I don't think its worth changing now because it should mean re-ordering the declarations again to keep them in alphabetical order. If it is ok with everyone merge with these names now and I will sort out the naming conventions when I do the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tjj2017 I have a small number of queries that are also just minor nitpicks / request for clarification (often just due to my poor knowledge of ADA) that I think should not get in the way of merging what is a great body of work.
I'll ask about these separately - if additional comments are helpful then they can be in a separate minor PR.

@xbauch xbauch force-pushed the do_cu_specs branch 2 times, most recently from 89c994a to 34cd22b Compare January 24, 2019 15:50
Copy link
Collaborator

@tjj2017 tjj2017 left a comment

Choose a reason for hiding this comment

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

Thank you for the change of comment. I am happy for you to merge.

@@ -226,6 +226,17 @@ package body Driver is
end;

if not Add_Start then
Copy link
Contributor

Choose a reason for hiding this comment

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

Default mode "don't add comments" to this thing I had to figure out what it is doing by looking at the the out parameter in another function.
Status: "on".
:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and noted for the next time.

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.

If everyone is happy; let's do this.

tjj2017 and others added 5 commits January 28, 2019 14:55
Introduces Register_Subprogram_Specification procedure that creates a new symbol
table entry with the subprogram name.
Process_Declaration(s) mirror the behaviour of Process_Statement(s), i.e.
a case analysis on the kind of declaration stored in the node being processed.
This commit also extends the cases in Process_Statement.
Introduces:
1) Do_Package_Declaration
2) Do_Package_Specification
-- processes private and visible declarations as a new code block
3) Do_Withed_Unit_Spec
-- case analysis for registering program specification/processing subprogram
declarations/package declarations
4) Do_Withed_Units_Specs
Also includes cprover_start even when there is no function to call in a CU.
Plus one more failing test (package function bodies are not processed).
@chrisr-diffblue chrisr-diffblue merged commit 56f1226 into diffblue:master Jan 28, 2019
@xbauch xbauch deleted the do_cu_specs branch March 14, 2019 10:20
@tjj2017 tjj2017 mentioned this pull request Apr 18, 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.

5 participants