search

google / wuffs

Wrangling Untrusted File Formats Safely
Other
4.16k stars 131 forks source link

wuffs-c requires clang-format-5.0 #10

Closed mvdan closed 6 years ago

mvdan commented 6 years ago

Is there a reason why version 5.0 is explicitly required? I only have clang-format installed (version 6.0), and creating a symlink in my $PATH keeps wuffs-c running normally.

I presume that the tool is used to format the C output from the generator. Why not make it configurable though? It could, for example, default to clang-format, where one could change it to clang-format-5.0 or any other desired version.

mvdan commented 6 years ago

Perhaps a relevant bit of info here is that I don't care about the output format, at least for now - I don't commit the output C files into my repository. I'll likely start doing that at some point, especially with Go code to keep it go-gettable, but for now I don't pay attention to formatting issues.

nigeltao commented 6 years ago

Why is an explicit version required? Why clang-format-5.0 and not just clang-format?

In the Wuffs repo (but not your repo), everything checked in under gen/c is clang-formatted. Different versions of clang-format will format the same C code differently. While the gen/c changes in https://github.com/google/wuffs/commit/abdf46045f1e7dd5d3a51ccdcc0f58ebbad8b96c are possibly a clang-format bug, it does illustrate my point. Without an explicit version, two people (or the same person working on two different systems) could cause spurious edits (and edit-revert wars) when checking in their changes.

Why version 5.0? Why not 4.0 or 6.0?

It's somewhat arbitrary, but 5.0 seemed a reasonable compromise of being not too old and not too new, as of today. A future change will undoubtedly bump 5.0 to something higher, whether 5.x or y.0.

nigeltao commented 6 years ago

Having said that, it'd be possible to make it configurable...

mvdan commented 6 years ago

Fair enough - thanks for the explanation. Having it configurable would be neat, but it's not a big deal for me now. I can work around it with a symlink easily enough :)