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)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaas, Assigned: wtc)

References

()

Details

Attachments

(1 file, 1 obsolete file)

httpget.c is really hard to read because of indentation and tabs
Attached patch fix indentation, remove tabs (obsolete) — Splinter Review
Attachment #211040 - Flags: superreview?(wtchang)
That patch only changes whitespace.
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-
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+
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
Sounds good. Thanks!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: