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

Refactor clang_getUnqualifiedType (#202) #203

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Conversation

TravisCardwell
Copy link
Collaborator

Motivation: perform checks in Haskell to take advantage of type safety.

  • WrapperResult is completely factored out.
  • ClangVersionError is added to indicate version support errors.
  • InvalidCXTypeError is added to indicate Haskell-side invalid type errors.
  • clang_getUnqualifiedType (Haskell) is refactored to check the libclang version and type validity, removing the checks from wrap_getUnqualifiedType (C).

@TravisCardwell TravisCardwell requested a review from edsko October 4, 2024 01:24
@TravisCardwell
Copy link
Collaborator Author

Build failures! Interesting. Builds and tests pass locally, and I purposefully made the build fail before changing the C code, so I do not think it is a local cache issue. Investigating...

@edsko
Copy link
Collaborator

edsko commented Oct 4, 2024

Build failures! Interesting. Builds and tests pass locally, and I purposefully made the build fail before changing the C code, so I do not think it is a local cache issue. Investigating...

Which OS do you use? On Ubuntu at least it's very easy to have multiple llvm verisons installed (I'm on Ubuntu 24, which supports llvm-14 through llvm-18). I switch between them by having something like this in my .envrc

# export PATH=/usr/lib/llvm-14/bin:$PATH

Not strictly needed, CI will check it anyway, and you'll probably not have to deal with different llvm versions very often anyway, but I thought I'd mention it just in case it's useful.

Motivation: perform checks in Haskell to take advantage of type safety.

* `WrapperResult` is completely factored out.
* `ClangVersionError` is added to indicate version support errors.
* `InvalidCXTypeError` is added to indicate Haskell-side invalid type
  errors.
* `clang_getUnqualifiedType` (Haskell) is refactored to check the
  `libclang` version and type validity, removing the checks from
  `wrap_getUnqualifiedType` (C).
@TravisCardwell TravisCardwell force-pushed the clang_getUnqualifiedType branch from 7945c8b to 5415b88 Compare October 7, 2024 01:28
@TravisCardwell
Copy link
Collaborator Author

TravisCardwell commented Oct 7, 2024

I am currently using Arch. llvm14 is available, so I installed it and confirmed that putting the directory at the beginning of my PATH works. Thank you!

Details
$ export PATH=/usr/lib/llvm14/bin:$PATH
$ rm -rf dist-newstyle
$ cabal build all
...
checking for llvm-config... /usr/lib/llvm14/bin/llvm-config
checking LLVM version... 14.0.6
checking LLVM library directory... /usr/lib/llvm14/lib
checking LLVM include directory... /usr/lib/llvm14/include
...

At some point, we might want to test against more versions. Debian has packages for many versions (on different distributions), and it should not be difficult to test locally within containers.

llvm Version Distribution
6 6.0.1 buster (oldoldstable)
7 7.0.1 buster (oldoldstable)
9 9.0.1 bullseye (oldstable)
11 11.0.1 bullseye (oldstable)
13 13.0.1 bookworm (stable)
14 14.0.6 bookworm (stable)
15 15.0.6 bookworm (stable)
16 16.0.6 bookworm (stable)
17 17.0.6 sid (unstable)
18 18.1.8 sid (unstable)
19 19.1.1 sid (unstable)
20 20240921 (experimental)

I am not sure what the situation is on Ubuntu, used in CI, as Ubuntu Packages Search is returning internal server errors when searching.

@edsko
Copy link
Collaborator

edsko commented Oct 8, 2024

Yes, testing with some older versions especially would be useful, to see if there are any assumptions that we are making that aren't warranted.

@edsko edsko merged commit b57b034 into main Oct 8, 2024
7 checks passed
@edsko edsko deleted the clang_getUnqualifiedType branch October 8, 2024 12:03
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.

2 participants