| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@nodejs/cpp-reviewers |
Sorry, something went wrong.
|
It's deliberately restrictive, always has been. It forces you to write more succinct code. Longer line lengths optimize for writability, shorter line lengths for readability. Code is read more often than it is written. |
Sorry, something went wrong.
|
My main concern right now is the format-cpp task. I've had to update PRs countless times because format-cpp wanted me to rewrite surrounding, existing code to match its code style. At least, once code has been committed, it won't have formatting issues. Now I am wondering if changing this rule will negatively impact progress there. |
Sorry, something went wrong.
Signed-off-by: James M Snell <jasnell@gmail.com>
I'm not convinced it would be an issue. At least, so far the experience with the quic pr has been pretty painless with this. I'd suggest we could go with this as an experiment. Rolling it back would be fairly straightforward if necessary. |
Sorry, something went wrong.
There was a problem hiding this comment.
I mulled this over but I really don't like flowery prose - you know the kind, interminable run-on sentences that take up half a page - and the same goes for code. Hemingway > *
Sorry, something went wrong.
|
Added tsc-agenda for discussion. If there are technical reasons not to make the change, that's one thing, but the objections raised so far appear purely subjective. @tniessen raised a possible technical issue that bears consideration. |
Sorry, something went wrong.
|
just for the sake of discussion, I disagree with
in many occasions reading the beginning of a line can spare you from reading the entire line. for example, SomeClass::SomeMethod(with, a, list, of, arguements); is more readable than SomeClass::SomeMethod(with,
a,
list,
of,
arguements);
especially when SomeClass::SomeMethod is named in a good and descriptive name |
Sorry, something went wrong.
|
Case in point, in the quic PR, I had to shorten... int err = nghttp3_conn_add_write_offset(connection_.get(), stream_data->id, datalen);to int err = nghttp3_conn_add_write_offset(connection_.get(),
stream_data->id,
datalen);I find the shorter lengths far less readable than the longer alternate. |
Sorry, something went wrong.
Looking at it in isolation in the GitHub UI, the single line is more readable. As someone who exclusively works on a small screen and frequently looks at git diffs in my terminal, I disagree that it's more readable while I'm trying to do actual work though. |
Sorry, something went wrong.
I dislike restricting the line length to 80 characters in general, but nonetheless the research that has been done bears out what Ben said: Keeping the lines to 80 chars or less improves readability. Granted, almost everyone who cites the research cites (I think) a single study (but multiple papers) by Buse & Weimer. So you might argue that the data on this topic is thin. You might also argue that a study done in 2008 may not reflect the reality of modern code bases. You might also argue that maintainability is different (albeit related) to readability and this improves maintainability because (for example) we don't have to reformat lines copied/rebased/cherry-picked/whatever from other code bases. And so on. But for Ben's point, what data there is backs up Ben on this as far as I am aware. Keeping most lines shorter than 80 characters correlates strongly with code readability (although there were some other elements that correlated more strongly in Buse & Weimer's research). And I say that as someone who dislikes 80 character line length limits. (100 or 120 seems reasonable to me.) |
Sorry, something went wrong.
|
We went through this whole debate for JavaScript in #41509 and #41586. I think the C++ rule should just match the JS one, where we settled on 120 characters (for now). I would also support upping the limit on Markdown either to match or for Markdown to have no limit. |
Sorry, something went wrong.
I mean, of course - the compiler doesn't care about line length but I, as someone who reviews a lot of code, definitely do. W.r.t. #45033 (comment)
For me it's completely the other way around. My eyes just kind of start to glaze over 2/3rds in. |
Sorry, something went wrong.
@jasnell Just to clarify my (possibly unfounded) concern: C++ PRs have often been much larger than necessary recently to pacify format-cpp, which requires surrounding lines to be formatted properly. That means that a one line diff can turn into reformatting an entire function. (Because make format-cpp does not work nicely in some git workflows, I tend to postpone it and then have to update the PR just to pacify format-cpp.) I am wondering if this change would cause more such unnecessarily large diffs because clang-format might want to unwrap lines that fit into the new limit but not into the old. |
Sorry, something went wrong.
|
@tniessen ... ah, yeah, that's a good point. I had no intention of making it so that existing code would need to change but adjusting the formatter would absolutely have that impact... and for that reason alone I'll withdraw this. |
Sorry, something went wrong.
|
(On the other hand, if this is something that we'd want to do eventually, I don't know if waiting is going to give us any benefit.) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
The existing 80 has become rather restrictive.