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

prefer aspects over representation clauses #72

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

Conversation

pjljvandelaar
Copy link
Contributor

Dear Gnatcoll Developers,

The code currently mixes aspects and representation clauses.
For readability only aspects are prefer.

This pull request fixes the issues.

Greetings,
Pierre

P.S. Not all code adheres to the setting of the pretty printer in project file, hence some changes are due to pretty printing.

Problem detected and solved by Rejuvenation-Ada crate
vote for Rejuvenation-Ada as The 2022 Ada Crate Of The Year

@CLAassistant
Copy link

CLAassistant commented Dec 15, 2022

CLA assistant check
All committers have signed the CLA.

@pjljvandelaar
Copy link
Contributor Author

In the past, TNO ESI has signed the Contributor License Agreement, so I don't understand why that isn't working anymore.

Don't understand why appveyor crashes. Did I cause that?

@t-14
Copy link
Collaborator

t-14 commented Dec 16, 2022

Can't say what's your past CLA status, can you just sign it again please?

Don't worry about the failing CI jobs, they are broken and due for a cleanup.

Copy link
Collaborator

@t-14 t-14 left a comment

Choose a reason for hiding this comment

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

Please remove the code reformatting which is problematic and anyway not an objective of this PR.

@pjljvandelaar
Copy link
Contributor Author

pjljvandelaar commented Dec 20, 2022

When aspects are combined (as is the case in this PR),
we need to be certain that the generated/rewritten code is valid.
Validity includes adhering to the pretty printer settings: many projects treat style violations as errors.

Limiting pretty printing to the rewritten code only is impossible with gnatpp:

  • comment is used to turn pretty printing on and off
  • comment is line based

Yet many AST nodes are only a part of a line.

When code adheres to the pretty print settings in the project file, pretty printing a slightly larger section is no problem.

So code that is pretty printed is violating your own pretty print settings!
So it is the boy-scout rule: leave the code better than you found it...

P.S. If you update your pretty print settings, I can rerun the analysis and rewritting.

P.S.2. Since it is a single file: I propose to solve it here manually: just remove the sections you don't want to change from the patch file.

@t-14
Copy link
Collaborator

t-14 commented Dec 20, 2022

So it is the boy-scout rule: leave the code better than you found it...

Not sure I follow. Do you really insist the following leaves the code better than you found it (or has anything to do with aspects vs replcauses)?

-           (Integer'Min (Max_Block_Len, 75)
-            - Block_Prefix'Length - Block_Suffix'Length) / 4) * 3;
+           (Integer'Min (Max_Block_Len, 75) - Block_Prefix'Length -
+            Block_Suffix'Length) /
+           4) *
+        3;

@pjljvandelaar
Copy link
Contributor Author

Not sure I follow. Do you really insist the following leaves the code better than you found it (or has anything to do with aspects vs replcauses)?

-           (Integer'Min (Max_Block_Len, 75)
-            - Block_Prefix'Length - Block_Suffix'Length) / 4) * 3;
+           (Integer'Min (Max_Block_Len, 75) - Block_Prefix'Length -
+            Block_Suffix'Length) /
+           4) *
+        3;

I agree you can agree about whether it is an improvement,
yet according to the settings for pretty printer, Adacore's gnatpp considers this the most desirable code.

@t-14
Copy link
Collaborator

t-14 commented Jan 11, 2023

yet according to the settings for pretty printer, Adacore's gnatpp considers this the most desirable code

Despite the word "pretty" in the name, gnatpp is really like black - i.e. it's about source uniformity rather than prettiness. We have no plans to enforce gnatpp's formatting for gnatcoll source at this point.

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.

3 participants