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

Clang format #6

Closed ckaran closed 4 years ago

ckaran commented 8 years ago

This may be a headache. I've discovered that sometimes the code passes all tests, and sometimes it doesn't even though no changes were made to the code. I strongly suspect that there is a reference to uninitialized memory somewhere that sometimes works and sometimes doesn't. As a test do the following:

make && make test
git clean -xdf
make && make test

If you get the same results I do, then the first time will fail, but the second time will succeed. This is fairly consistent, which means that bad things are happening somewhere deep in the code. I don't know if you want to reject this request, but I do suggest grabbing a copy of my repository and seeing if you get the same results as I am.

ckaran commented 8 years ago

@jplevyak You can probably merge this into your code at this point. The only file that I'm having a hard time with is write_tables.c; everything else works. In particular, I was able to kill off the missing initializers warnings, which means that the tests are now deterministic. I also was able to regenerate the committed grammar.g.c file from the built make_dparser executable in the directory, so everything seems to be working reasonably well. Would you be willing to work on getting write_tables.c working with clang-format? It's the last C file that can't be fed through it safely, and I'm going to be too busy for the next while to look at it.

ckaran commented 8 years ago

OK, I figured out what was causing the hangups with write_tables.c, and fixed them. As a result clang-format now works; I ran it on all .h and .c files, and ran all the tests.

In addition, I add a .clang-tidy file for clang-tidy, and ran it on all the .c files, so all the statements that didn't have braces now do. That is:

if (a)
    return blah;

is now

if (a)
{
    return blah;
}

That said, there are still a lot of warnings when you use -Wextra -pedantic -c11 as the set of flags. I don't have time right now to go through all of them, and this branch isn't the right place for them anyways (it should have only contained clang-format related code). Once you've pulled all the outstanding pull requests, can you let me know? I'll delete all those branches and get to work on the other stuff. Until then I'm going to work on other projects.

jplevyak commented 8 years ago

I'll put it under valgrind and see if I can find it.

On Oct 26, 2016 12:28 PM, "ckaran" notifications@github.com wrote:

This may be a headache. I've discovered that sometimes the code passes all tests, and sometimes it doesn't even though no changes were made to the code. I strongly suspect that there is a reference to uninitialized memory somewhere that sometimes works and sometimes doesn't. As a test do the following:

make && make test git clean -xdf make && make test

If you get the same results I do, then the first time will fail, but the second time will succeed. This is fairly consistent, which means that bad things are happening somewhere deep in the code. I don't know if you want to reject this request, but I do suggest grabbing a copy of my repository

and seeing if you get the same results as I am.

You can view, comment on, or merge this pull request online at:

https://github.com/jplevyak/dparser/pull/6 Commit Summary

  • Added .clang-format file.
  • Repaired permissions.
  • Removed self-assignment.
  • Merge branch 'fix_permissions' into clang_format
  • Merge branch 'fix_self_assignments' into clang_format
  • Used clang-format to format all .h files.
  • Merge branch 'master' into clang_format
  • Added .gitignore file.
  • Merge branch 'add_gitignore' into clang_format
  • Applied clang-format -i to dparse_tree.h
  • Ran 'clang-format -i' on driver_parsetree.c
  • Ran 'clang-format -i' on gram.c
  • Ran 'clang-format -i' on lr.c
  • Merge https://github.com/jplevyak/dparser into clang_format
  • Merge https://github.com/jplevyak/dparser into fix_self_assignments
  • Merge branch 'fix_self_assignments' into clang_format
  • Added idempotency locks and 'extern C' checks.
  • Ran clang-format -i on the files in this commit.
  • Ran 'clang-format -i' on util.c; the output now passes all tests.
  • Merge branch 'master' into add_gitignore
  • Merge branch 'add_gitignore' into clang_format
  • Ran 'clang-format -i' on grammar.g.c, and it passes tests now.
  • Changed compiler in Makefile from gcc to clang.
  • Started work on cleaning up write_tables.c with clang-format.
  • More formatting of write_tables.c.
  • More formatting work on write_tables.c
  • More formatting of write_tables.c
  • More formatting.
  • More formatting.

File Changes

Patch Links:

— 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/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AARLoBwXYmtOusFD7TvHGy5vVmWG2l16ks5q36m7gaJpZM4Khkfc .

jplevyak commented 8 years ago

IMO brackets should follow the if:

if (a) { foo(); }

and single lines should follow as well without brackets:

if (a) foo();

ckaran commented 8 years ago

I see where you're coming from, but I disagree. I've had a few too many cases switching between Python and C where I forget that whitespace doesn't indicate which statement a block is a part of. Explicitly putting in braces everywhere really makes it clear what a block belongs to.

As for the brace following the if, or being on a separate line, I personally prefer it on a separate line, but I can edit the .clang-format file so that it puts it on the same line. Do you want me to go ahead and do so, or do you want to do it?

Also, has anything shaken out of valgrind yet?

jplevyak commented 4 years ago

Formatted in a different commit.