search

jplevyak / dparser

A Scannerless GLR parser/parser generater.
https://github.com/jplevyak/dparser
BSD 3-Clause "New" or "Revised" License
105 stars 14 forks source link

Added .clang-format and applied the clang-format tool to all .c and .… #1

Closed ckaran closed 7 years ago

ckaran commented 7 years ago

Added .clang-format and applied the clang-format tool to all .c and .h files.

clang-format is a tool for automatically formatting C/C++/Obj-C code. This allows a project to define a formatting style that is applied across the entire project, which can make it much easier to read. I've added the .clang-format file at the root of the project which can be modified to control the styles used. I've also formatted all .c and .h files using clang-format and the style in this commit.

ckaran commented 7 years ago

OK, this is the first time I've had clang-format emit failing code. I'm going to withdraw this pull request and work on figuring out what went wrong.

jplevyak commented 7 years ago

There are a couple issues. The first is that this is old C code and so it doesn't obey the typical modern rules, in particular all the headers are not #ifdef protected, and do not include their dependencies, instead there is a single header "d.h" which everyone is supposed to include which handles the dependencies explicitly. This is a problem because clang-format likes to reorder headers alphabetically. The generated code also uses #ifndef XXX to allow XXX to be overridden which also depends on header order. The second problem is that add_array_member is using stringification in the preprocessor with strings of the form 0x%x which clang-format incorrectly interprets as 0x % x instead of as a token which will be stringified.

These issues could be fixed. The latter is a bit annoying because it is a arguably a bug in clang-format.

On Wed, Oct 19, 2016 at 7:11 AM, ckaran notifications@github.com wrote:

OK, this is the first time I've had clang-format emit failing code. I'm going to withdraw this pull request and work on figuring out what went wrong.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jplevyak/dparser/pull/1#issuecomment-254824389, or mute the thread https://github.com/notifications/unsubscribe-auth/AARLoCM9JtYfP1TDHNqW9R3Ywlr21Dz0ks5q1iT9gaJpZM4KbAP3 .

ckaran commented 7 years ago

That would explain quite a bit. I've started working on individual files, getting them through clang-format.

I could also make modifications to the code outside of clang-format, but that will mean that the branch will have a LOT of weird intermingled changes that aren't related to clang-format directly. Will that bother you? If not, I'll keep on banging on this branch until it all works.

jplevyak commented 7 years ago

It doesn't bother me if they are clean(ish). This is old code so it could benefit from the help.

BTW, the "fn = fn;" was to avoid the unused parameter warning. There is also a warning for leaving off the parameter name in C, so no help there.

I changed it to (void)fn; which is more C standard.

I also removed the tabs and trailing spaces from that file and pushed it before I knew you were still banging on the formatting.

Sorry if that is going to cause some merge issues.

On Tue, Oct 25, 2016 at 1:04 PM, ckaran notifications@github.com wrote:

That would explain quite a bit. I've started working on individual files, getting them through clang-format.

I could also make modifications to the code outside of clang-format, but that will mean that the branch will have a LOT of weird intermingled changes that aren't related to clang-format directly. Will that bother you? If not, I'll keep on banging on this branch until it all works.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jplevyak/dparser/pull/1#issuecomment-256159721, or mute the thread https://github.com/notifications/unsubscribe-auth/AARLoMyELN5k6-HEFMsSAbm8GVy0tMI8ks5q3mDCgaJpZM4KbAP3 .

ckaran commented 7 years ago

No problem, I went ahead and merged in the changes.

On a separate note, is it OK if we switch to using clang as the compiler? The only copy of gcc that I have is ancient, and I can't easily upgrade it currently, but I have a modern version of clang (I know that is a poor excuse for switching compilers, but I'm really kind of stuck right now).

jplevyak commented 7 years ago

Sure. It should work with both. Make it use CC in the environment if it is available.

On Wed, Oct 26, 2016 at 7:12 AM, ckaran notifications@github.com wrote:

No problem, I went ahead and merged in the changes.

On a separate note, is it OK if we switch to using clang http://clang.llvm.org/ as the compiler? The only copy of gcc that I have is ancient, and I can't easily upgrade it currently, but I have a modern version of clang (I know that is a poor excuse for switching compilers, but I'm really kind of stuck right now).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jplevyak/dparser/pull/1#issuecomment-256359778, or mute the thread https://github.com/notifications/unsubscribe-auth/AARLoC_Hs1d8pCG4JxEvE1zjbOIaDF6kks5q31_hgaJpZM4KbAP3 .

ckaran commented 7 years ago

Actually, my plan is just to make the change temporarily; assuming you're OK with it, I'd like to move it to using cmake so that we can get build files for multiple build systems (I personally prefer ninja because of how fast it is, and cmake generates ninja files). That move will be in another branch at another time though.

BTW, I vaguely remember seeing a comment in the code saying that using tables were slow, and that there were some speedups that you were planning on including. Is that still a part of the plan?