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

(WIP) Add a simple check against functions containing both return; and return <obj>; #466

Closed
wants to merge 1 commit into from

Conversation

stevelinton
Copy link
Contributor

Adds a simple check as discussed. Ignores extra return added at the end of functions.

Throws up many warnings in library and packages.

warns.txt

@olexandr-konovalov
Copy link
Member

That's quite illuminating, and the failed travis test is easily fixable.

@olexandr-konovalov olexandr-konovalov added the gapsagedays2016 Issues and PRs that arose at https://www.gapdays.de/gap-sage-days2016 label Jan 15, 2016
@olexandr-konovalov
Copy link
Member

In the first two randomly picked up cases, I've seen a redundant return; after Error -

  if not IsSemigroupCongruence(cong) then
    Error("usage: the argument should be a semigroup congruence,");
    return;
  fi;
  • maybe most of these cases fall under the same pattern.

@ChrisJefferson
Copy link
Contributor

I believe (I have said this before) that most cases like that should be changed to ErrorMayQuit, which ensures users can't type return in the break loop which is what this return is there for, but that would be a separate patch.

@fingolfin
Copy link
Member

Awesome. And the very first warning actually looks like an actual error (not just a case where Error should be replaced by ErrorMayQuit. I think getting fixing all these warnings, and merging this patch, would be worthwhile.
Of course not for 4.8 (but since we already branched stable-4.8, that should be clear anyway)

@fingolfin
Copy link
Member

I have updated and extended this in PR #1541, so closing this PR now.

@fingolfin fingolfin closed this Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapsagedays2016 Issues and PRs that arose at https://www.gapdays.de/gap-sage-days2016
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants