Closed
Bug 326283
Opened 18 years ago
Closed 18 years ago
httpget.c is really hard to read because of indentation and tabs
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.7
People
(Reporter: jaas, Assigned: wtc)
References
()
Details
Attachments
(1 file, 1 obsolete file)
593 bytes,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
httpget.c is really hard to read because of indentation and tabs
Attachment #211040 -
Flags: superreview?(wtchang)
Assignee | ||
Comment 3•18 years ago
|
||
Comment on attachment 211040 [details] [diff] [review] fix indentation, remove tabs In the NSPR source tree, the default indentation level is 4, the default tab stop is 8, and tabs may be used in place of 8 spaces. This file follows that policy. The only thing that needs fixing is the emacs or vim directive at the top of the file: /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Attachment #211040 -
Flags: superreview?(wtchang) → superreview-
Assignee | ||
Comment 4•18 years ago
|
||
I don't use any editor that parses this directive, so I am not sure if it is right for NSPR's default policy of indentation level = 4 and tab stop = 8. These editor directives were added to NSPR when the Netscape browser source code was open sourced in 1998. Since none of the NSPR developers use an editor that parses these directives, we never know if they are right. Also, some of the NSPR files use a tab stop of 4, so we can't simply change these directives the same way.
Attachment #211040 -
Attachment is obsolete: true
Attachment #211051 -
Flags: review?(joshmoz)
Do you allow tabs in nspr? Also, see the following lines: 204 if (addr == (void *) -1) { 205 fprintf(stderr, "cannot memory-map file: (%d, %d)\n", PR_GetError(), 206 PR_GetOSError()); 207 208 PR_CloseFileMap(outfMap); 209 return PR_FAILURE; 210 } What makes this hard to read is that there is no indentation within the |if| block. Is this really valid nspr coding style? Stuff like this happens all over the file. I don't think fixing the editor directive is going to make this any more readable to 90% of those who might read it.
I understand that tabs are supposed to be 8 spaces, but unless you set up your editor this way it doesn't help much. Even lxr doesn't work that way. Can we at least get rid of tabs (replace each one with 8 spaces) in addition to fixing the editor directive? I realize this isn't the place to debate coding style for the nspr codebase, and I don't want to do that anyway :)
Attachment #211051 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 7•18 years ago
|
||
Josh, in a new project, I would ban tabs where they aren't required. I worked on a product that disallowed CVS checkins that contained tabs before, so this issue is not new to me. But NSPR is a finished product and our focus now is to maintain it. So we have different requirements, such as minimizing diffs. By the way, the default tab stop in vi, Notepad (Windows), and TextEdit (Mac OS X) is 8. In any case, last night I looked at several NSPR files in LXR and found that LXR seems to parse the Emacs mode line. So I checked in the "fix the editor directive" patch (on NSPR trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH) as an experiment. This morning I checked the LXR. Indeed, httpget.c looks right now. So if any other NSPR file doesn't look right in LXR, we should first see if that can be fixed by changing the Emacs mode line.
Severity: minor → trivial
Status: NEW → RESOLVED
Closed: 18 years ago
OS: MacOS X → All
Hardware: Macintosh → All
Resolution: --- → FIXED
Target Milestone: --- → 4.7
You need to log in
before you can comment on or make changes to this bug.
Description
•