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

Avoid warnings instead of hiding them #2873

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Avoid warnings instead of hiding them #2873

merged 2 commits into from
Feb 5, 2025

Conversation

kleinreact
Copy link
Member

@kleinreact kleinreact commented Jan 13, 2025

Certain GHC warnings are currently disabled in in Clash.Prelude to hide the fact some imports and bindings are not used, and that some of the patterns in Clash.Sized.Vector are incomplete. As this is avoidable though, and having those flags enabled may also hide problems that are easy to catch otherwise. Therefore, this PR just avoids production of the respective warnings in the first place.

The only current exception is Clash.Tutorial, where the unused imports are used to have shorter Haddock references. We also can avoid that one by using fully qualified imports for the references instead. I'm in favor of introducing those changes as part of this PR too, but please let me know first, whether that's of common interest, as it takes a few minutes to apply all the changes along the file.

Requires (parts of)

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@kleinreact kleinreact force-pushed the avoid-warnings branch 3 times, most recently from afee508 to c1c1517 Compare January 29, 2025 18:28
Copy link
Member

@DigitalBrains1 DigitalBrains1 left a comment

Choose a reason for hiding this comment

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

Woops, my comment directly above was meant to be part of a review.

Speaking in general, I'm all in favour of this effort and I thank you for fiddling with it and trying to get it all working.

But I do have some comments regarding documentation and documentation-related subjects.

clash-prelude/src/Clash/Examples/Internal.hs Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Member

Why did you include part of PR #2843 here? Is there something blocking the merge of PR #2843?

@kleinreact
Copy link
Member Author

Why did you include part of PR #2843 here?

@DigitalBrains1 The version of select from #2843 is just much better suited for getting rid of the incomplete patterns, which is why I just cherry picked that version to avoid unnecessary extra work.

Is there something blocking the merge of #2843?

Not from my side. Shall I merge it?

@DigitalBrains1
Copy link
Member

Shall I merge it?

Point 1: definitely not without rebasing first, after this long a time you really can't trust that that old CI success is still valid!

Point 2: I think it needs approving reviews first. Rowan and I both raised issues, you resolved the issues, but I don't see any approving reviews yet. I think the only thing that's keeping me personally from an approving review, after I've now re-studied it, is that I didn't check yet whether all the changed code is well covered in unit tests and/or HDL tests. It looks fine, but does it work fine? :-D

So perhaps I need to go hunt for existing tests and then either approve the PR or ask you to produce tests if they are not well covered.

@kleinreact kleinreact merged commit fbd486e into master Feb 5, 2025
13 checks passed
@kleinreact kleinreact deleted the avoid-warnings branch February 5, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants