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

New: clashCompileError #2399

Merged
merged 1 commit into from
Jan 11, 2023
Merged

New: clashCompileError #2399

merged 1 commit into from
Jan 11, 2023

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Jan 6, 2023

Based upon the version proposed by @basile-henry in issue #2020:
#2020 (comment)

Still TODO:

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

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

LGTM (assuming copyright / CI fixes). As far as the clash-dev failure goes: it seems to happen sporadically and I can't reproduce it locally. Given that it is not anything user-facing I'd be fine with disabling the check for 9.2.

clash-lib/src/Clash/Primitives/Prelude.hs Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Member Author

GHC 9.2 complaining about incomplete pattern matches (which actually pointed out a real bug in my code) made me realise it should probably do better on error reporting in the case a user passes some string which cannot be reduced to a literal by Clash. So I did some improvements there. I also trimmed the Changelog entry a bit; I can be a bit of a blabbermouth.

@DigitalBrains1
Copy link
Member Author

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

I'd like to have the error "foo" instead of undefined, but otherwise LGTM. Feel free to ignore/incorporate the other suggestion.

clash-lib/src/Clash/Primitives/Prelude.hs Outdated Show resolved Hide resolved
clash-lib/src/Clash/Primitives/Prelude.hs Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Member Author

Ah, actually using TermLiteral changes things a lot, I think it's an improvement. Do you agree?

@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Jan 8, 2023

Ah, it can be even shorter; that does produce a useless call stack as part of the error but the code is so nice and simple this way.

[edit]
This is the message with the new implementation:

<no location info>: error:
    Clash error call:
    clashCompileError: <your message here>
    CallStack (from HasCallStack):
      error, called at src/Clash/Netlist/Util.hs:876:17 in clash-lib-1.7.0-inplace:Clash.Netlist.Util

Previously the last two lines were not included by virtue of errorWithoutStackTrace
[/edit]

@martijnbastiaan
Copy link
Member

Definitely an improvement!

Based upon the version proposed by @basile-henry in issue #2020:
#2020 (comment)
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.

None yet

2 participants