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

use short-circuit where possible #73

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

Conversation

pjljvandelaar
Copy link
Contributor

Dear Gnatcoll developers,

According to adaic
short circuit operators should be preferred.
However, in case of side effects in the second/right argument the replacement is NOT equivalent.

The rejuvenation library has detected where logical operators can be replaced by short-circuit ones
and replaced them.

Greetings,
Pierre

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

Added a second commit that prefers membership test.
Unfortunately I could not make a separate pull request for it.

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 code reformatting, and preserve formatting logic in sections that are being refactored.

@@ -1695,27 +1685,14 @@ package body GNATCOLL.Email.Utils is

if Set = Charset_US_ASCII then
Encoding := Encoding_7bit;
elsif Set = Charset_ISO_8859_1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you want this code to be rewritten?
given that you want remove code reformatting, and preserve formatting logic in sections that are being refactored.

Note that the newly rewritten code must be pretty printed,
since otherwise the rewritten code might violate style rules (as specified in the project file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case a too long line is certainly possible ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am referring to places where reformatting is not motivated by anything, like gnatcoll-memory.adb:291

Copy link
Collaborator

@t-14 t-14 Dec 20, 2022

Choose a reason for hiding this comment

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

How would you want this code to be rewritten?

In this specific case, try to keep the same terms on each line as it was previously. We used to have one line for latin-3 and another for latin-4, your patch reformats it so references to latin-4 are on two different lines, this harms readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You did however not turn off pretty printing,
so running the pretty printer might have changed your constraints.

It is an automated change, but it is a nice case of requirements for rewriting existing code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You did however not turn off pretty printing,

I don't understand. "turn off pretty printing" where? gnatcoll is obviously hand-formatted.

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