Re: [dev] dmenu_path rewrite in C

On 05/19/2010 02:24 PM, pancake wrote:
> 1)
> In unix there's a MAXPATH variable.. in fact GNU does not have this limit,
> but in unix is 4096 and in plan9 256 (afaik)

Actually, PATH_MAX.

> the thing is that keeping a clean system you shouldn't have paths that big.

agreed

> So you can define a single buffer of this size to strcpy/memcpy/strcat the
> paths you need to construct the executable paths you need.
>
> this will reduce the heap usage a lot.

This approach would also add complexity. I would guess that disk IO is
the limiting factor to the speed, not malloc.

> the memory accesses will be hardly reduced because you can keep the basedir
> on the buffer on all iterations for each directory. only changing filename.

Hmm, this is a good point. I think I'll implement (at least some of) it.

> 2)
> syntax is not following the suckless style, I would prefer to have all source
> files in suckless following the same rules.

That's easy to fix, and arguably unnecessary.

> 3)
> There'r many places where you don't check for result of malloc/getenv is null.

Now I do. Haven't finished the cleanup yet, though.

> 4)
> many variables can be removed (like copy_path in get_PATH()

The question is not whether they can be removed. SHOULD they be removed,
this is the question for me.

> 5)
> I would add 'die()' instead of perror+exit

Done

> 6)
> use sizeof(buf) instead of hardcoded size (makes the code safer to changes)

Fixed

> 7)
> I would change --force flag check to be just '-f'

A matter of taste.

> 8)
> why do you check for root?

Why do you use dmenu_path as root? I can't see any use cases for this.

> 9)
> as all filepaths are of similar size you can just allocate blocks of this size
> and index by using a multiplier which results faster than having many chunks
> (with some tests i did in 'alt' (hg.youterm.com/alt) this kind of optimizations
> result in 3-5x faster execution.

And would also add much confusion to the reader (and, judging by the
mailing list, our readers seem to have sensitive eyes).

> 10)
> put ".dmenu_cache" path as define on top of C file. so you can change it easily.

Full path? No thanks, it better sit at $HOME.
Received on Wed May 19 2010 - 13:11:15 UTC