httpget.c is really hard to read because of indentation and tabs

RESOLVED FIXED in 4.7

Status

NSPR
NSPR
--
trivial
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Josh Aas, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
httpget.c is really hard to read because of indentation and tabs
(Reporter)

Comment 1

12 years ago
Created attachment 211040 [details] [diff] [review]
fix indentation, remove tabs
Attachment #211040 - Flags: superreview?(wtchang)
(Reporter)

Comment 2

12 years ago
That patch only changes whitespace.
(Assignee)

Comment 3

12 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

12 years ago
Created attachment 211051 [details] [diff] [review]
fix the editor directive

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)
(Reporter)

Comment 5

12 years ago
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.
(Reporter)

Comment 6

12 years ago
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 :)
(Reporter)

Updated

12 years ago
Attachment #211051 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 7

12 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
Last Resolved: 12 years ago
OS: MacOS X → All
Hardware: Macintosh → All
Resolution: --- → FIXED
Target Milestone: --- → 4.7
(Reporter)

Comment 8

12 years ago
Sounds good. Thanks!
You need to log in before you can comment on or make changes to this bug.