Eclipse is very nice, but :(...

Michael Halcrow mike at halcrow.us
Tue Jun 20 16:48:51 MDT 2006


On Tue, Jun 20, 2006 at 12:44:23PM -0600, Hans Fugal wrote:
> Let's just use occam's razor and keep it simple, folks. Tabs are 8
> spaces. If you want something else, use spaces.

While we're all on the pedantical sled...

http://en.wikipedia.org/wiki/Occam%27s_razor

``Furthermore, when multiple competing theories have equal predictive
powers, the principle recommends selecting those that introduce the
fewest assumptions and postulate the fewest hypothetical entities. It
is in this sense that Occam's razor is usually understood.''

> While I'm here I might as well weigh in on the rest of the
> opinions. :) I like 4 spaces in C-like languages, 2 spaces in ruby
> or XML. I don't buy the "8-space tabs in C if you can't read it
> you're an idiot programmer" argument,

If you have trouble fitting your code into 80 columns with 8-space
tabs, then more likely than not, your are trying to do too much in a
single function, and your code's maintainability suffers. Or you are
missing a wonderful opportunity to introduce a well-placed goto.

> I find that frequently it takes nearly 60 characters to write a
> decent line of code using some APIs or well-named variables, and
> breaking lines is almost always less readable than not breaking
> them, so 8-space indentation makes a real mess sooner rather than
> later.

The Linux kernel code base begs to differ.

The 8-space indentation helps discipline the developer to break his
code into sane and manageable segments. You will see that there are
occasional violations of the 80-column rule in the kernel code, and
one might argue that these violations help readability in those cases,
but the vast majority of code one writes should stick to the rule.

I have managed to write 6,000+ lines of code implementing a
cryptographic filesystem (see fs/ecryptfs in the -mm tree) whilst
following the 80-column rule 100%. In hindsight, I wish I had broken
that rule in a few places, but it can be done, and the code can be
left in a very readable state. Here is one instance that sticks out to
me on a second glance (converting tabs to spaces in order to emphasize
the point to those with broken non-8-space-tabs mail readers, although
those with variable-width font mail readers are totally screwed up
anyway, which is a topic for another flamefest):

---
        while (index < new_end_page_index) {
                /* Fill all intermediate pages with zeros */
                rc = write_zeros(file, index, 0, PAGE_CACHE_SIZE);
                if (rc) {
                        ecryptfs_printk(KERN_ERR, "write_zeros(file=[%p], "
                                        "index=[0x%.16x], "
                                        "old_end_pos_in_page=[d], "
                                        "(PAGE_CACHE_SIZE - new_end_pos_in_page"
                                        "=[%d]"
                                        ")=[d]) returned [%d]\n", file, index,
                                        old_end_pos_in_page,
                                        new_end_pos_in_page,
                                        (PAGE_CACHE_SIZE - new_end_pos_in_page),
                                        rc);
                        goto out;
                }
                index++;
        }
---

Yuck. I should have just run past the end on that one. But the rest of
the code is left in decent shape. For instance:

---
static int ecryptfs_readpage(struct file *file, struct page *page)
{
        int rc = 0;
        struct ecryptfs_crypt_stat *crypt_stat;

        BUG_ON(!(file && file->f_dentry && file->f_dentry->d_inode));
        crypt_stat =
                &ecryptfs_inode_to_private(file->f_dentry->d_inode)->crypt_stat;
        if (!crypt_stat
            || !ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_ENCRYPTED)
            || ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_NEW_FILE)) {
                ecryptfs_printk(KERN_DEBUG,
                                "Passing through unencrypted page\n");
                rc = ecryptfs_do_readpage(file, page, page->index);
                if (rc) {
                        ecryptfs_printk(KERN_ERR, "Error reading page; rc = "
                                        "[%d]\n", rc);
                        goto out;
                }
        } else {
                rc = ecryptfs_decrypt_page(file, page);
                if (rc) {

                        ecryptfs_printk(KERN_ERR, "Error decrypting page; "
                                        "rc = [%d]\n", rc);
                        goto out;
                }
        }
        SetPageUptodate(page);
out:
        if (rc)
                ClearPageUptodate(page);
        ecryptfs_printk(KERN_DEBUG, "Unlocking page with index = [0x%.16x]\n",
                        page->index);
        unlock_page(page);
        return rc;
}
---

Okay; that's not so great either. I really need to do something about
the overly-verbose filesystem object interpolation calls (i.e.,
ecryptfs_inode_to_private). But that's just about the right size for a
function, it stays within 3 indentations, the ||'d values in the
conditional are broken up into their own lines, and it is pretty
maintainable. Looking back, perhaps it would be best to just let most
of the printk's run past the end rather than breaking the quoted
material in awkward places.

Well, you get the idea. There is a very good reasons for requiring
developers to stick with 8 tabs/80 columns in general, not the least
of which is to encourage the developer to keep the code broken up into
concise functional units that are easily understandable and
maintainable.

I have to go with the strategy of the kernel maintainers on this
one. If you wanted to submit code to a project that I maintained, I
would reject your patch if you wrecklessly disregarded the 80-column
rule with 8-space tabs.

Mike
.___________________________________________________________________.
                         Michael A. Halcrow                          
       Security Software Engineer, IBM Linux Technology Center       
GnuPG Fingerprint: 419C 5B1E 948A FA73 A54C  20F5 DB40 8531 6DCA 8769

If this is flashing then you're a winner!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 481 bytes
Desc: Digital signature
Url : http://plug.org/pipermail/plug/attachments/20060620/45c72d21/attachment.bin 


More information about the PLUG mailing list