Explicit error for comments, with README link by cellularmitosis · Pull Request #569 · eliben/pycparser

cellularmitosis · 2025-04-01T10:13:27Z

@eliben thanks so much for your blog and for pycparser!

This PR adds an explicit exception for comments, with a hint to the user that clang's cpp does not remove comments, and that simply using gcc's cpp instead will likely solve their problem.

$ cat foo.c
int foo() {
    // this is a comment.
    return 42;
}

$ python3 examples/c_json.py foo.c
Traceback (most recent call last):
pycparser.plyparser.ParseError: foo.c:2:5: Comments are not supported (note: GCC's cpp removes comments, Clang's cpp does not).

See also my comment on a related github issue.

Hopefully this will curb the endless flood of "comments don't work" github issues! 😭

eliben

Thanks. I'm still thinking about this, but here are some initial comments

Also: add tests

eliben · 2025-04-01T12:35:41Z

    bad_octal_constant = '0[0-7]*[89]'

+    # comments are not supported
+    unsupported_c89_comment = r'\/\/'


I don't understand the c89/c99 distinction here. Aren't // just C++ style comments and /* the original C style?

I thought it made more sense to describe the comments from a C perspective, rather than a C++ perspective. I'll update this.

Also, I accidentally swapped c89 / c99.

eliben · 2025-04-01T12:36:35Z


+    @TOKEN(unsupported_c89_comment)
+    def t_UNSUPPORTED_C89_COMMENT(self, t):
+        msg = "Comments are not supported (note: GCC's cpp removes comments, Clang's cpp does not)."


This error message is way too specific; you have a particular issue you're encountering now, but there are a million other reasons people could try to run pycparser on files with comments. It could point to the repository's README, for example (which has a section on the preprocessor)

Thanks for the context, I'll point to the README, but I'd also like to add more context to the README.

Is his diagram an accurate understanding of the situation?

cellularmitosis · 2025-04-02T05:27:54Z

A bit more context on my findings: invoking clang's preprocessor as cpp appears to have different behavior than invoking it as clang -E. In particular, macOS system headers appear to rely on clang extensions, but apparently cpp turns support for them off.

In other words, clang's/usr/bin/cpp chokes on any macOS system header, while clang -E works (but does not remove comments).

cellularmitosis · 2025-04-02T05:47:30Z

PR updated, tests are still TODO.

(Edit: added a second commit rather than amend / force-push, to preserve PR context)

cellularmitosis · 2025-04-02T06:10:54Z

Ok, tests added to PR.

This is ready for a re-review.

eliben · 2025-04-02T12:50:11Z

+``cpp``. A compatible ``cpp`` handles preprocessing directives like ``#include`` and
 ``#define``, removes comments, and performs other minor tasks that prepare the C
-code for compilation.
+code for compilation. Note that clang's cpp does not remove comments.


I'm not sure what Clang's cpp is... not clang -E, right? Regardless, such a comment will likely go out of date, and I prefer excluding it. The sentence above says enough about what cpp should do

eliben · 2025-04-02T12:53:01Z

A bit more context on my findings: invoking clang's preprocessor as cpp appears to have different behavior than invoking it as clang -E. In particular, macOS system headers appear to rely on clang extensions, but apparently cpp turns support for them off.

In other words, clang's/usr/bin/cpp chokes on any macOS system header, while clang -E works (but does not remove comments).

OK, thanks I understand the context now.

Still, I wouldn't say too much about this in the README. I understand you ran into a frustrating situation, but we shouldn't over-index on it. pycparser has been around for ages now, and this very rarely comes up. The errors you added should improve the situation significantly, but I don't want to maintain additional statements and flowcharts on the README that may go stale at any point

cellularmitosis · 2025-04-02T13:39:15Z

Sorry for the confusion: the flowchart was just for communication during this PR and will not be introduced into the repo.

I picked up on and am trying hard to satisfy the desire to avoid over-indexing on error specificity, but I'm going to be frank: I feel the current PR's user help on this topic is already too minimal.

The current PR:

adds the phrase "A compatible"
adds the sentence "Note that clang's cpp does not remove comments."

To communicate my thinking on this topic, here's a possible spectrum of context we could offer the user:

The minimum context the user needs in order to understand (but not fix) the problem is:

parse_file() and preprocess_file() require a cpp which strips comments
your cpp is not stripping comments

The minimum context to fix the problem would also include:

gcc is an example of a cpp which strips comments

However, I also feel that the majority of developers are on Macs these days. A pleasant experience would also include direct, specific advice for that majority:

brew install gcc
add cpp_path='/opt/homebrew/bin/cpp-14' as a parameter to any parse_file() or preprocess_file() calls

Please understand that the user help in the current PR is already less that what I'd prefer.

cellularmitosis · 2025-04-02T13:42:53Z

Thinking about the staleness issue a bit more, I could change "Note that clang's cpp does not remove comments" to something like "Gcc includes a cpp which meets these requirements".

That would be less likely to go stale, as it is independent of clang's behavior, and I would guess it is very unlikely that gcc would ever change their implementation to stop stripping comments.

eliben · 2025-04-02T13:45:20Z

Thanks for sharing your thoughts. The PR's largest change is adding an explicit error "comments are not supported" with a README link. A user following this link will read about the requirements of cpp and will notice their cpp doesn't do the job. The text also says one may use clang -E instead of cpp, and even points to an example.

I really think this is sufficient for now.

By the way, it's entirely possible that Mac's cpp not removing comments is a new thing, because this issue would have come up more much frequently otherwise. These things change from time to time, and I want to keep our documentation as resilient as possible to these changes (it already talks quite a bit about the cpp issue!!)

cellularmitosis · 2025-04-02T13:48:58Z

Oh my gosh, I am so sorry, I somehow missed that while clang's cpp does not remove comments, clang -E does remove comments.

cellularmitosis · 2025-04-02T14:00:05Z

Ok, PR updated to remove "Note that clang's cpp does not remove comments".

Explicit error for comments, with user hint

f915e6a

cellularmitosis mentioned this pull request Apr 1, 2025

Parsing '//' comments #464

Closed

eliben reviewed Apr 1, 2025

View reviewed changes

cellularmitosis force-pushed the unsupported-comments branch from f915e6a to a3b0bba Compare April 2, 2025 05:46

naming, README

06589e7

cellularmitosis force-pushed the unsupported-comments branch from a3b0bba to 06589e7 Compare April 2, 2025 05:53

adding tests

2599bbc

cellularmitosis changed the title ~~Explicit error for comments, with user hint~~ Explicit error for comments, with README link Apr 2, 2025

eliben reviewed Apr 2, 2025

View reviewed changes

Remove reference to current clang behavior

7b7dae8

eliben approved these changes Apr 2, 2025

View reviewed changes

eliben merged commit 156eae7 into eliben:main Apr 2, 2025
15 checks passed

never147 mentioned this pull request Oct 28, 2025

Bump pycparser from 2.22 to 2.23 never147/ansible-desktop#62

Merged

Conversation

cellularmitosis commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eliben left a comment

Choose a reason for hiding this comment

Uh oh!

eliben Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

cellularmitosis Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

eliben Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

cellularmitosis Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

cellularmitosis commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cellularmitosis commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cellularmitosis commented Apr 2, 2025

Uh oh!

eliben Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

eliben commented Apr 2, 2025

Uh oh!

cellularmitosis commented Apr 2, 2025

Uh oh!

cellularmitosis commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eliben commented Apr 2, 2025

Uh oh!

cellularmitosis commented Apr 2, 2025

Uh oh!

cellularmitosis commented Apr 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cellularmitosis commented Apr 1, 2025 •

edited

Loading

cellularmitosis commented Apr 2, 2025 •

edited

Loading

cellularmitosis commented Apr 2, 2025 •

edited

Loading

cellularmitosis commented Apr 2, 2025 •

edited

Loading