vickenty / lang-c

Lightweight C parser for Rust
Apache License 2.0
202 stars 30 forks source link

Support macOS and clang #3

Closed fotonick closed 5 years ago

fotonick commented 5 years ago

I wanted to try your library on a Mac, but was stymied by two issues:

  1. cpp is broken on macOS in that it's both stuck in K&R mode and has a bug surrounding its handling of indented preprocessor directives. Ref: Stack Overflow. To this end:
    1. I submit a patch to use clang -E when target_os = macos.
    2. Incidentally, if target_os is not linux or macos, I throw a compile error. Thus Windows is explicitly not yet supported (this project's only open issue).
    3. I only tested on macOS 10.14.2, as I don't have a Linux box in a working state at the moment, so I hope you'll help me fill in the testing there.
  2. Once you pre-process with Clang on macOS, trivial programs parse fine, but as soon as you import any standard headers, you get a SyntaxError. In the case of stdio.h, it's because it uses a new type qualifier _Nullable, which is not defined elsewhere. It turns out that it's a Clang extension. To address this, I added some Clang support:
    1. I added the nullability qualifiers to the grammar.
    2. I listed all the Clang-specific keywords I could find in strings.rs.
    3. Clang seems to support an almost super-set of the gcc extensions, so I added a new Flavor::Clang11 and reworked the Env initialization to make "core" C11 a subset of gnu11 and gnu11 a subset of Clang11.
    4. I added one test for the grammar additions and the new Env option.
    5. What's missing: I didn't add the other keywords words to the grammar. These include things like controlling OpenCL address spaces and some Windows-specific MS extensions. I also didn't update the readme. I figure you would want to choose what to say, if anything.

I hope you'll consider taking these patches. I'm very open to feedback, particularly as I'm somewhat new to Rust.

vickenty commented 5 years ago

First of all, thank you for the PR, and for the detailed description!

I'm very happy to add clang support, even if partial. There are a couple of nits that I'd like to see fixed before merging, though, I'll comment on them inline. But otherwise nice job.

I'm also okay with defaulting to clang on macos (AIUI, clang comes with XCode, but GCC has to be installed manually). By the way, does gcc -E work on MacOS instead of cpp?

I'm not so sure about limiting library to linux and macos. Besides windows, there are various other operating systems that Rust supports, and at least the various BSD variants would have either gcc or clang. I don't know what is the best course of action here yet, but at the very least I'd leave cpp as a fallback.

fotonick commented 5 years ago

Thanks for the comments. They seem like strict improvements. Will get to them in the next day or so. MacOS does make the gcc alias by default, so gcc -E could have broad applicability. Maybe broader than cpp?

fotonick commented 5 years ago

I don't know if Github notifies you about file updates. Regardless, it's worth saying that I've taken a stab at addressing all of your comments, so it's worth reevaluating whenever you're ready.

vickenty commented 5 years ago

Looks good, thank you!