Reformat code (and add make target to do so) #326

sbarzowski · 2017-07-20T16:56:50Z

No description provided.

sbarzowski · 2017-07-20T17:08:38Z

it's a massive change, even though I tried to match the settings with the existing style. There were no manual changes (it's a bad idea to mix automatic reformatting with other restructuring imo).

Sometimes it goes from bad to worse, for example when there are plenty of arguments to a function. But I think it's still a net win, especially if we enforce it automatically later on.

@lwander, @sparkprime

lwander

Looks largely good to me, my only gripe is the comment doc reformatting - I think that's something to fix before merging.

Also, this might be the right time to introduce a check in our Travis CI build to require code to match the output of make reformat.

lwander · 2017-07-20T17:12:01Z

Makefile

@@ -101,6 +101,9 @@ test: jsonnet libjsonnet.so libjsonnet_test_snippet libjsonnet_test_file
 	cd test_suite ; ./run_tests.sh
 	cd test_suite ; ./run_fmt_tests.sh

+reformat:


all should probably depend on this.

also I think you should declare this as .PHONY

I'm worried that reformatting on every build (== adding to all) may be annoying.

👍 for .PHONY I forgot about it.

Also I guess I'll add a separate test-formatting target for use in TravisCI etc.

test-formatting added, ..PHONY added

My concern with all is not really valid, there is a separate default target. But still I interpret all as build all as it doesn't run tests etc., so I think reformatting doesn't really belong there. @sparkprime?

I don't think make all should change source code. However it can run the test-formatting.

lwander · 2017-07-20T17:12:34Z

core/static_analysis.cpp

- * \param vars The variables defined within lexical scope of ast_.
- * \returns The free variables in ast_.
+ * \param in_object Whether or not ast_ is within the lexical scope of an object
+ * AST. \param vars The variables defined within lexical scope of ast_. \returns


This kind of doc reformatting seems wrong

I think I can fix it through CommentPragma option, thanks for noticing

lwander · 2017-07-20T17:13:06Z

.clang-format

@@ -0,0 +1,108 @@
+---
+Language:        Cpp
+# BasedOnStyle:  Google


Why commented out?

The real reason is that it was autogenerated this way. But I guess I can either remove it completely or leave it active. I'll take another look at this config.

sparkprime · 2017-07-20T18:23:26Z

#143

@davidzchen will be happy about this

sparkprime · 2017-07-20T20:22:14Z

.clang-format

+SpacesInParentheses: false
+SpacesInSquareBrackets: false
+Standard:        Auto
+TabWidth:        8


Should this be 4?

No, this is not the indentation, but how we interpret actual tab characters (we change them to this many spaces then). For example Google style, that I started with, uses 8 here, even though the Indentation is only 2 chars wide.

sbarzowski · 2017-07-21T15:35:40Z

BTW if we want to enforce it automatically there are some workflow issues to discuss. Most importantly the problems with having different versions of clang-format on different machines which may disagree with each other and the "official" version on TravisCI. The most proper solution I think is to pin down the version of clang-format somehow, locally for the project. I was using a workflow like this very successfully in Python and it was very easy with virtualenv. Some people do it for C++, but it's quite complex (https://engineering.mongodb.com/post/succeeding-with-clangformat-part-2-the-big-reformat). I suppose typically C++ projects use autoformatting in a more ad-hoc way.

But probably it's a topic for another change/issue.

sparkprime · 2017-07-21T17:33:35Z

Did you try it with 100 line width yet?

sparkprime · 2017-07-21T17:38:25Z

This doesn't seem too onerous:

The Wrapper Script

In any non-trivial use of clang-format, you will want a wrapper around it. Our script, clang_format.py:

Fetches clang-format binaries, from LLVM when it can, and from a MongoDB cache when it cannot
Validates clang-format is at the right version when binaries are already present
Skips files that we shouldn’t bother formatting, i.e. 3rd party library code, like boost
Offers modes for both validating and updating
Manages parallel invocations of clang-format to increase throughput

sbarzowski · 2017-07-21T18:20:29Z

Did you try it with 100 line width yet?

Yes, current version uses 100 as a target width.

sparkprime · 2017-07-21T18:45:18Z

core/lexer.h

@@ -56,14 +56,14 @@ static inline std::ostream &operator<<(std::ostream &o, const FodderElement &f)
 {
    switch (f.kind) {
        case FodderElement::LINE_END:
-        o << "END(" << f.blanks << ", " << f.indent << ", " << f.comment[0] << ")";
-        break;
+            o << "END(" << f.blanks << ", " << f.indent << ", " << f.comment[0] << ")";


I have a moderate preference for keeping them on the same line, because they're essentially just indirect goto labels. However if there's no supported option in clang for that then I can live with it.

I checked that and I couldn't find anything to control that.

sparkprime · 2017-07-21T18:45:43Z

core/lexer.h

-            case TRUE: return "true";
-
-            case END_OF_FILE: return "end of file";
+            case BRACE_L:


Any way to encourage it to put the return on the same line? This is less readable than before.

Yes I experimented with that, I think there were fewer changes if I set it this way, but I might be wrong. Anyway, I'll change it.

sparkprime · 2017-07-21T18:46:43Z

core/lexer_test.cpp

-void testLex(const char* name,
-             const char* input,
-             const std::list<Token>& tokens,
+namespace


I've never seen anyone put the { after a namespace before

(on the line after)

sparkprime · 2017-07-21T18:49:27Z

core/libjsonnet.cpp

-        abort(); \
+#define CATCH(func)                                                                           \
+    }                                                                                         \
+    catch (const std::bad_alloc &)                                                            \


I think try { } catch should have the same rules as if { } else { }

It does. But here we have crazy macros, so it gets a little confused.

sparkprime · 2017-07-21T18:50:02Z

core/libjsonnet.cpp

@@ -277,16 +284,14 @@ const char *jsonnet_version(void)

 JsonnetVm *jsonnet_make(void)
 {
-    TRY
-    return new JsonnetVm();
+    TRY return new JsonnetVm();


we could change this to

TRY { return new JsonnetVm(); } CATCH("jsonnet_make")

It'll probably behavior better then.

(move the { } out of the macro, that is)

I was able to tell clang-format that TRY ... CATCH is a block (as it is, no changes to the macro).

sparkprime · 2017-07-21T18:52:18Z

core/libjsonnet.cpp

-        std::string input;
-        input.assign(std::istreambuf_iterator<char>(f),
-                     std::istreambuf_iterator<char>());
+    TRY std::ifstream f;


TRY { would fix this indentation too

sparkprime · 2017-07-23T00:32:46Z

cmd/jsonnet.cpp

-            // TODO(dcunnin): Remove deprecated --var, --env and -E
-            } else if (arg == "-V" || arg == "--ext-str"
-                       || arg == "--var" || arg == "--env" || arg == "-E") {
+                // TODO(dcunnin): Remove deprecated --var, --env and -E


Maybe we should do this: #327

Yes, that will help.

sparkprime · 2017-07-23T00:34:11Z

core/lexer.cpp

-            case '\t':
-            indent += 8;
-            break;
+            case '\t': indent += 8; break;


weird that it put it on the one line for this case but not the ' ' case above

clang-format can sometimes produce weird results. Some quick experimentation shows that the reason why the ' ' case is not put in one line is the comment below it. But I have no idea why.

sparkprime · 2017-07-23T00:36:23Z

core/lexer.cpp

+                    case '6':
+                    case '7':
+                    case '8':
+                    case '9': state = AFTER_DIGIT; break;


Any way to get it to binpack the cases?

I can't find any.

sparkprime · 2017-07-23T00:43:57Z

Ok I read the whole thing. Generally this is a big net improvement. The majority of changes are actually good, the rest are things that don't make it better / worse but were not consistent across the whole codebase originally. Probably I'd just do something random in each case. The only thing that's still worth looking into is the bin packing of cases in that floating point number lexing rule. However that's not a deal breaker by any means. Thanks for setting this up!

sbarzowski · 2017-07-24T20:23:05Z

(rebased)

googlebot added the cla: yes label Jul 20, 2017

lwander reviewed Jul 20, 2017

View reviewed changes

sparkprime reviewed Jul 20, 2017

View reviewed changes

sbarzowski force-pushed the reformat branch from ff941bb to b6fa0ad Compare July 20, 2017 21:06

sbarzowski changed the title ~~Refromat code (and add make target to do so)~~ Reformat code (and add make target to do so) Jul 20, 2017

sparkprime reviewed Jul 21, 2017

View reviewed changes

sparkprime reviewed Jul 23, 2017

View reviewed changes

sbarzowski force-pushed the reformat branch from bbacfd7 to 5561bef Compare July 24, 2017 20:21

sbarzowski force-pushed the reformat branch from 5561bef to bc69b0d Compare July 26, 2017 20:18

Reformat code (and add make target to do so)

9f5697c

sbarzowski force-pushed the reformat branch from bc69b0d to 9f5697c Compare August 2, 2017 15:30

sparkprime merged commit 03e0c92 into google:master Aug 3, 2017

Reformat code (and add make target to do so) #326

Reformat code (and add make target to do so) #326

Conversation

sbarzowski commented Jul 20, 2017

sbarzowski commented Jul 20, 2017

lwander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparkprime commented Jul 20, 2017

Choose a reason for hiding this comment

sbarzowski Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

sbarzowski commented Jul 21, 2017 • edited Loading

sparkprime commented Jul 21, 2017

sparkprime commented Jul 21, 2017

sbarzowski commented Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbarzowski Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparkprime commented Jul 23, 2017

sbarzowski commented Jul 24, 2017

sbarzowski Jul 20, 2017 •

edited

Loading

sbarzowski commented Jul 21, 2017 •

edited

Loading

sbarzowski commented Jul 21, 2017 •

edited

Loading

sbarzowski Jul 21, 2017 •

edited

Loading