Github reorganization

zebra · 2016-03-25T01:12:23.868Z

Definitely want an 80 char line limit?

@sam, sorry, i think i misunderstood - you would like a longer line limit, yeah? fine with me.

seems like if we’re gonna impose a format, now would be the time to do it. i’ll open a PR for libavr32 with clang-format and people can weight in on tweaks for it

[edit] … aaand, actually, i don’t know. it seems like clang-format just doesn’t handle aligning #defines. i can’t make it not do this:

-#define BANG        (0x1E)
-#define AT          (0x1F)
-#define POUND       (0x20)
-#define DOLLAR      (0x21)
-#define PERCENT     (0x22)
-#define CAP         (0x23)
-#define AND         (0x24)
-#define STAR        (0x25)
-#define OPENBKT     (0x26)
-#define CLOSEBKT    (0x27)
+#define BANG (0x1E)
+#define AT (0x1F)
+#define POUND (0x20)
+#define DOLLAR (0x21)
+#define PERCENT (0x22)
+#define CAP (0x23)
+#define AND (0x24)
+#define STAR (0x25)
+#define OPENBKT (0x26)
+#define CLOSEBKT (0x27)

anyone know a way around?
(SO doesn’t: c++ - Can clang-format align variable or macro assignments in columns? - Stack Overflow )

or, maybe i should be looking for a different formatter (uncrustify?); this is kind of a deal-breaker.

sam · 2016-03-26T08:58:49.975Z

Right, after banging my head repeatedly against a wall for 2 days trying to get avr32-toolchain to build properly on newer versions of Linux (there are issues with newer versions of gperf and texinfo), I gave up and just used the Atmel builds. Maybe when I stop being quite so fed up with it, I’ll write it up as an issue on the repo.

Anyway, the main point being, I’ve now got travis-ci builds running, I’ve just created 2 so far, a simple one for the teletype repo: https://travis-ci.org/samdoshi/teletype

And a more complicated one for the libavr32 repo: https://travis-ci.org/samdoshi/libavr32

The libavr32 one will download the master branch of each of the firmwares, copy itself over and test to see if anything breaks. It could be useful for PRs.

The downside, particularly for the libavr32 repo, is that we’re using submodules, we don’t need each firmware to work with the latest libavr32. There’s no point using a CI if you don’t expect it to always pass. IMO having a CI that is expected to fail is worse than no CI.

Anyway, let me know if you’re interested in getting travis-ci working on the repos and I’ll create some PRs. We’ll need to move this repo over to the monome org too.

ngwese · 2016-03-26T15:33:59.843Z

sam:

Right, after banging my head repeatedly against a wall for 2 days trying to get avr32-toolchain to build properly on newer versions of Linux (there are issues with newer versions of gperf and texinfo), I gave up and just used the Atmel builds. Maybe when I stop being quite so fed up with it, I’ll write it up as an issue on the repo.

That brings back memories, sure enough I did run into that problem… from my post about aleph toolchain hosts way back when:

Aleph toolchain hosts - Mac or Linux?

So I switched to trying Ubuntu 14.04. Getting the precompiled blackfin toolchain running was easy but it took a lot of trial and error / hacking to get the avr32 toolchian compiled as above. In the end I had to build an older version of texinfo (version 4.13) and hack one of the files generated by gperf (removing inline in one place) to get things

sam · 2016-03-26T16:09:11.454Z

There is this fork of the avr32-toolchain repo: https://github.com/lis-epfl/avr32-toolchain

They’ve got patches to fix gperf and textinfo changes, they build on my mac, (and bizarrely give some extra warnings when compiling our code). But I couldn’t get them to build on the travis-ci OSX images.

I’ll try to update the readme later this week with updated Linux recommendations.

zebra · 2016-03-26T21:30:52.496Z

for what it’s worth, i’ve never had much luck with the jsnyder toolchain scripts on linux, nor had much motivation to try very hard since the atmel releases work fine.

the whole strategy of the Makefile seems kinda… brittle. combining specific arbitrary versions of gcc boostrap sources, with basically random versions of tools like texinfo…

sam · 2016-03-27T15:55:35.474Z

It’s handy because it does build a usable toolchain on OSX, which is very convenient. But I agree the Makefile is really hard to deal with. When it stops working I’ll replace it with the Atmel binaries and Vagrant / Docker / whatever the flavour of the month is.

@tehn, any interest in the getting travis set up?

https://travis-ci.org/samdoshi/libavr32/builds/118831379

tehn · 2016-03-28T21:30:09.412Z

sam:

any interest in the getting travis set up?

at this point i’m still coming to grips with the submodule workflow-- minor confusion being worked out right now in fact while adding clock division to MP (which requires abstracting the external clock, which is a good thing in general).

i’ll check out travis!

sam · 2016-04-03T15:38:11.508Z

zebra:

or, maybe i should be looking for a different formatter (uncrustify?); this is kind of a deal-breaker.

I need to get the teletype codebase cleaned up a bit before I work on it, I’m going slightly crazy trying to follow control flow with the mixed up indentation.

So I was thinking, it doesn’t have to be a continuous affair, we could just take the hit once, sort the #defines out, and then all just try hard to keep it tidy looking. Thoughts?

@tehn, shall I just get on with it and run clang-format against the teletype codebase?

tehn · 2016-04-03T16:53:44.326Z

sam:

So I was thinking, it doesn’t have to be a continuous affair, we could just take the hit once, sort the #defines out, and then all just try hard to keep it tidy looking. Thoughts?

@tehn, shall I just get on with it and run clang-format against the teletype codebase?

i say go. i can help sort out the messy things by hand as well (ie #defines)-- i’m pretty fast with sublime hotkeys

zebra · 2016-04-03T20:34:30.945Z

Works for me … 20

ppqq · 2016-04-05T04:55:29.252Z

zebra:

anyone know a way around?

Is wrapping #defines in // clang-format off and // clang-format on an option? (Does that even work?)

EDIT: more on clang ignores in the docs.

zebra · 2016-04-05T06:13:14.567Z

Yeah, that occurred to me, it is a little irritating to have to baby the formatter that way. Good point though; probably better than having to fix things by hand.

sam · 2016-04-05T14:28:15.640Z

github.com/monome/teletype

.editorconfig & .clang-format

by samdoshi on 02:27PM - 05 Apr 16 UTC

3 commits changed 10 files with 3747 additions and 3504 deletions.

ppqq · 2016-04-06T16:24:53.199Z

sam:

Anyway, let me know if you’re interested in getting travis-ci working on the repos and I’ll create some PRs. We’ll need to move this repo over to the monome org too.

Late to the party but yes please!

scanner_darkly · 2017-06-22T23:45:28.077Z

working with teletype code right now and the 80 column clang format is driving me a bit crazy - any opposition to changing it to 100? or even 110/120?

tehn · 2017-06-23T00:26:40.147Z

i could be ok with 100, though i understand the style guideline for 80.

what editor do you use, by the way?

(i’ll generally defer to @sam on these matters)

ngwese · 2017-06-23T05:44:15.389Z

No opposition from me on switching to 100 columns.

sam · 2017-06-23T06:25:34.686Z

I vote no. But if everyone else wants to…

IIRC I tried reformatting the code to 100 columns when I was initially fiddling with clang-format and rejected it in favour of 80 columns, 100 columns is a bit too hard to use on a 13" laptop screen in my opinion (esp, if you want 2 windows side by side). Horizontal scrolling in code is an awful thing to have to do.

The biggest reason to say no, is that if we change it to 100, clang-format will make the following changes and mess up the commit history.

 module/edit_mode.c       |  42 ++----
 module/flash.c           |  21 ++-
 module/globals.h         |   9 +-
 module/help_mode.c       |  31 ++---
 module/keyboard_helper.h |  15 +--
 module/line_editor.c     |   3 +-
 module/line_editor.h     |   3 +-
 module/live_mode.c       |   9 +-
 module/main.c            |  22 +---
 module/pattern_mode.c    |  28 ++--
 module/preset_r_mode.c   |  14 +-
 module/preset_w_mode.c   |   4 +-
 module/usb_disk_mode.c   |   9 +-
 simulator/tt.c           |   4 +-
 src/command.c            |   7 +-
 src/ops/ansible.c        | 328 ++++++++++++++++++++-------------------------
 src/ops/controlflow.c    |  45 +++----
 src/ops/delay.c          |  13 +-
 src/ops/hardware.c       | 122 ++++++++---------
 src/ops/justfriends.c    |  42 +++---
 src/ops/maths.c          |  99 +++++---------
 src/ops/metronome.c      |  35 +++--
 src/ops/op.c             | 116 ++++++++--------
 src/ops/op.h             |  37 ++----
 src/ops/patterns.c       | 226 +++++++++++++++----------------
 src/ops/queue.c          |  39 +++---
 src/ops/stack.c          |  24 ++--
 src/ops/telex.c          | 483 +++++++++++++++++++++++++++++++------------------------------------
 src/ops/telex.h          |   3 +-
 src/ops/variables.c      |  42 +++---
 src/scanner.h            |   3 +-
 src/state.c              |  31 ++---
 src/state.h              |  26 ++--
 src/table.c              |  89 ++++++-------
 src/teletype.c           |  25 ++--
 src/teletype.h           |  12 +-
 tests/op_mod_tests.c     |  20 +--
 tests/parser_tests.c     |   3 +-
 tests/process_tests.c    |  10 +-

The original Teletype code base had too much nesting (sorry @tehn), keeping the lines short helps to prevent that IMO. Anyway it can’t be as bad as the Linux style guide, which is 80 columns wide, but with 8 character tabs…

@scanner_darkly is there something in particular that’s causing problems?

scanner_darkly · 2017-06-23T17:15:32.357Z

tehn:

what editor do you use, by the way?

i use notepad++, it has language specific formatting and colour coding / auto completion / plugins, and i just like it in general.

@sam - you suggested 100 upthread, curious what changed your mind? smaller screen argument makes sense (as it happens i’m planning to spend several hours coding on my 13" laptop, so i should be able to understand this concern better, especially since i do use 2 windows side by side all the time). but 80 still feels so limited. take this code for instance:

github.com

monome/teletype/blob/main/src/state.c#L217


      
          
          // Hardware
          
          void ss_set_in(scene_state_t *ss, int16_t value) {
              ss->variables.in = value;
          }
          
          void ss_set_param(scene_state_t *ss, int16_t value) {
              ss->variables.param = value;
          }
          
          void ss_set_scene(scene_state_t *ss, int16_t value) {
              ss->variables.scene = value;
          }
          
          uint8_t ss_get_script_pol(scene_state_t *ss, size_t idx) {
              return ss->variables.script_pol[idx];
          }
          
          void ss_set_script_pol(scene_state_t *ss, size_t idx, uint8_t value) {
              if (idx >= TRIGGER_INPUTS) return;

if the limit was 90 characters the method above wouldn’t need to use 2 lines for 207/208, 212/213, 218/219. to me breaking those lines in 2 is a poor trade off - that extra millisecond it takes when you just need to quickly scan some code adds up. and teletype code has a lot of examples like this, since there are many deep nested structures. op_ definitions is another example (i doubt you would need to check those declarations once they’re done, those really ought to be behind clang-format off i think…):

github.com

monome/teletype/blob/main/src/ops/ansible.c#L8


      
          #include "ops/ansible.h"
          
          #include "helpers.h"
          #include "ii.h"
          #include "teletype_io.h"
          
          
          static void op_ANS_G_LED_get(const void *data, scene_state_t *ss,
                                       exec_state_t *es, command_state_t *cs);
          static void op_ANS_G_get(const void *data, scene_state_t *ss, exec_state_t *es,
                                   command_state_t *cs);
          static void op_ANS_G_set(const void *data, scene_state_t *ss, exec_state_t *es,
                                   command_state_t *cs);
          static void op_ANS_G_P_get(const void *data, scene_state_t *ss,
                                     exec_state_t *es, command_state_t *cs);
          static void op_ANS_A_LED_get(const void *data, scene_state_t *ss,
                                       exec_state_t *es, command_state_t *cs);
          static void op_ANS_A_get(const void *data, scene_state_t *ss, exec_state_t *es,

re: git - why would it mess up the commit history? it can take longer to do a blame but you can still track any changes, no?

i’m okay with leaving it as 80 if you prefer (and several hours on a laptop might change my mind about increasing it), and i refactored some of my code to take advantage of #define to make some lines shorter, so it’s less of a problem now (and there is always clang-format off option for where it makes sense).

sam · 2017-06-23T18:05:46.516Z

scanner_darkly:

you suggested 100 upthread, curious what changed your mind?

I tried both of them out. 100 felt too wide, 80 seemed a lot nicer. Given that 80 char columns is still considered the ideal by cleverer people than me, I just went with the flow.

I’ve generally become a big fan of 80 char columns, I used to code in Visual Studio a lot, with ridiculously long lines of code and a tiny font. These days I much prefer a really big font and a narrow width, I find it a lot more relaxing.

Part of the issue here is that clang-format has a “take it or leave it attitude”, the example you gave could probably be formatted differently within the 80 characters and be more readable, but we don’t get that choice.

scanner_darkly:

it can take longer to do a blame but you can still track any changes, no?

I’m just talking about it messing up blame.

Blame is incredible useful, particularly when you’re new to a codebase, GitHub’s blame view particularly (at least it is to me). It shouldn’t be discounted how useful these tools can be to new contributors.

scanner_darkly:

i’m okay with leaving it as 80 if you prefer (and several hours on a laptop might change my mind about increasing it)

Have a go for a bit, particularly on the smaller screen. If you still feel really strongly about it, then the best I can suggest is we have a vote amongst the contributors. That way I can vote no and not feel quite so bad about saying no.

(or alternatively @tehn can pull BDFL privileges and just tell us what to do)